Skip to content

Add GRW automatic steps from shape #5690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 29, 2022
Merged

Add GRW automatic steps from shape #5690

merged 2 commits into from
Apr 29, 2022

Conversation

canyon289
Copy link
Member

Copies of intent of this commit 47571a7 from this PR #5541

@canyon289 canyon289 requested a review from ricardoV94 April 5, 2022 02:59
@codecov
Copy link

codecov bot commented Apr 5, 2022 •
edited
Loading

Codecov Report

Merging #5690 (7582cf8) into main (458f28e) will decrease coverage by 0.88%.
The diff coverage is 46.15%.

❗ Current head 7582cf8 differs from pull request most recent head 6978efb. Consider uploading reports for the commit 6978efb to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5690      +/-   ##
==========================================
- Coverage   88.90%   88.02%   -0.89%     
==========================================
  Files          75       75              
  Lines       13723    13750      +27     
==========================================
- Hits        12201    12103      -98     
- Misses       1522     1647     +125
Impacted Files Coverage Δ
pymc/distributions/timeseries.py 42.56% <46.15%> (-4.72%) ⬇️
pymc/model.py 78.99% <0.00%> (-7.64%) ⬇️
pymc/step_methods/hmc/quadpotential.py 73.56% <0.00%> (-6.99%) ⬇️
pymc/model_graph.py 82.06% <0.00%> (-3.45%) ⬇️
pymc/util.py 73.75% <0.00%> (-2.50%) ⬇️
pymc/distributions/discrete.py 98.13% <0.00%> (-1.60%) ⬇️
pymc/printing.py 85.14% <0.00%> (-1.00%) ⬇️
pymc/distributions/multivariate.py 91.20% <0.00%> (-0.82%) ⬇️
pymc/sampling.py 88.39% <0.00%> (-0.12%) ⬇️
... and 2 more

@ricardoV94 ricardoV94 changed the title Add GRW automatc steps selection from shape detection Add GRW automatic steps from shape Apr 5, 2022
ricardoV94
ricardoV94 requested changes Apr 5, 2022
View reviewed changes
@ricardoV94
Copy link
Member

Looks good, except the new ValueError is too restrictive

@ricardoV94 ricardoV94 mentioned this pull request Apr 8, 2022
Closed
@michaelosthege
Copy link
Member

@canyon289 can you push this PR over the finish line?

@ricardoV94
Copy link
Member

I am no longer sure about this. There are a couple of more special cases that would fail or redundant information would be ignored

  1. Ellipsis in the last shape entry
  2. Dims instead of shape
  3. Observed instead of shape

The first would fail with the new logic and has to be fixed. The others are unfortunate that we don't support as well.

ricardoV94
ricardoV94 requested changes Apr 16, 2022
View reviewed changes
@canyon289
Copy link
Member Author

How we feeling about this one?

@canyon289
Copy link
Member Author

CI is not feeling good about it 😭

I see the change is due to the recently added ellipsis changes, I wont be able to get to this for a while, a couple of weeks for sure, and then maybe after as well. If someone wants to take this over go for it, if not ill be back eventually

@ricardoV94 ricardoV94 force-pushed the grw_shape_steps_detection branch 2 times, most recently from 9c84422 to 6295cca Compare April 27, 2022 16:40
@ricardoV94
Copy link
Member

@canyon289 I force pushed some changes. Let me know if you spot any issues

@ricardoV94 ricardoV94 force-pushed the grw_shape_steps_detection branch from 6295cca to 2e1455e Compare April 27, 2022 16:41
@canyon289
Copy link
Member Author

Ill review in next 48 hours. Thanks Ricardo

@canyon289 canyon289 closed this Apr 27, 2022
@ricardoV94 ricardoV94 reopened this Apr 27, 2022
@ricardoV94 ricardoV94 force-pushed the grw_shape_steps_detection branch from 2e1455e to 6978efb Compare April 28, 2022 12:05
michaelosthege
michaelosthege approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I commented one detail, but go ahead however you like

pymc/distributions/timeseries.py
Comment on lines +175 to +178
if shape is not None:
shape = to_tuple(shape)
if shape[-1] is not ...:
steps_from_shape = shape[-1] - 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could break for shape=tuple()

...which is too low-dimensional to begin with.

@canyon289
Copy link
Member Author

Ah sorry for closing by accident

@ricardoV94
Copy link
Member

Merging this now as I will reuse this logic for the AR

@ricardoV94 ricardoV94 merged commit 6ae7628 into pymc-devs:main Apr 29, 2022
This was referenced May 5, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelosthege michaelosthege michaelosthege approved these changes

@ricardoV94 ricardoV94 ricardoV94 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@canyon289 @ricardoV94 @michaelosthege