Skip to content
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.

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

Obtain timeseries step information from dims or observed #5743

Merged
merged 3 commits into from
May 6, 2022

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented May 5, 2022

Follow up to #5690 so that steps can also be retrieved from observed and dims when possible.

Other changes:

  • Renamed pandas_to_array to convert_observed_data
  • Made AR steps more consistent with expectation / GRW

@ricardoV94 ricardoV94 force-pushed the shape_from_dims_observed branch 3 times, most recently from a7d9fde to b7a6290 Compare May 5, 2022 09:41
@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #5743 (504da82) into main (c76b9b9) will decrease coverage by 0.08%.
The diff coverage is 94.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5743      +/-   ##
==========================================
- Coverage   89.22%   89.13%   -0.09%     
==========================================
  Files          75       75              
  Lines       13824    13844      +20     
==========================================
+ Hits        12334    12340       +6     
- Misses       1490     1504      +14     
Impacted Files Coverage Δ
pymc/distributions/timeseries.py 77.55% <93.75%> (+1.27%) ⬆️
pymc/aesaraf.py 91.46% <100.00%> (ø)
pymc/data.py 79.83% <100.00%> (-3.79%) ⬇️
pymc/distributions/shape_utils.py 97.07% <100.00%> (ø)
pymc/model.py 86.00% <100.00%> (-0.55%) ⬇️

@ricardoV94
Copy link
Member Author

We really need to sort out what's going with #5739, it's failing nearly every time

This is consistent with the meaning of steps in the GaussianRandomWalk and translates directly to the number of scan steps taken
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 really good!
I particularly like the test cases.

@@ -245,6 +274,7 @@ def dist(
and init.owner.op.ndim_supp == 0
):
raise TypeError("init must be a univariate distribution variable")
check_dist_not_registered(init)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment what this line does?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the function self documents itself well enough:

pymc/pymc/util.py

Lines 337 to 351 in 02897d1

def check_dist_not_registered(dist, model=None):
"""Check that a dist is not registered in the model already"""
from pymc.model import modelcontext
try:
model = modelcontext(None)
except TypeError:
pass
else:
if dist in model.basic_RVs:
raise ValueError(
f"The dist {dist} was already registered in the current model.\n"
f"You should use an unregistered (unnamed) distribution created via "
f"the `.dist()` API instead, such as:\n`dist=pm.Normal.dist(0, 1)`"
)

@ricardoV94 ricardoV94 merged commit dd23c90 into pymc-devs:main May 6, 2022
@ricardoV94 ricardoV94 deleted the shape_from_dims_observed branch June 6, 2023 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants