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.

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

Record deterministic RandomVariables by default #148

Merged
merged 8 commits into from
Jun 4, 2024
Merged

Record deterministic RandomVariables by default #148

merged 8 commits into from
Jun 4, 2024

Conversation

damonbayer
Copy link
Collaborator

Closes #127

@damonbayer damonbayer linked an issue Jun 3, 2024 that may be closed by this pull request
All RandomVariables should be recorded (even if they are deterministic) #127
Closed
@codecov Codecov
Copy link

codecov bot commented Jun 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.93%. Comparing base (f11ce38) to head (a8b9fe6).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   93.90%   93.93%   +0.02%     
==========================================
  Files          36       36              
  Lines         706      709       +3     
==========================================
+ Hits          663      666       +3     
  Misses         43       43
Flag Coverage Δ
unittests 93.93% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dylanhmorris
Copy link
Collaborator

If record=True is going to be the default (and I think it should be, though I'm persuadable), then I think label should not have a default argument, as this will make it easy for the user to end up with variable name collisions, as occurs in the test suite here:

E       AssertionError: all sites must have unique names but got `a_random_variable` duplicated

Also, since the label attribute will be passed to numpyro.deterministic() as the name argument, I think it should be renamed name for consistency.

https://github.com/CDCgov/multisignal-epi-inference/blob/83dd9ddf11df1b0e5a5059f34fb7e1a84e466b9e/model/src/pyrenew/deterministic/deterministic.py#L17-L29

@damonbayer damonbayer marked this pull request as ready for review June 4, 2024 20:22
@damonbayer damonbayer force-pushed the dmb_127-all-randomvariables-should-be-recorded-even-if-they-are-deterministic branch from c5fcafb to 6559059 Compare June 4, 2024 20:36
@damonbayer
Copy link
Collaborator Author

@dylanhmorris @gvegayon Ready for review, but expecting many merge conflicts, depending on when other PR's land.

gvegayon
gvegayon approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@gvegayon gvegayon 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! One minor comment. Should we be more explicit about key word arguments? I feel like we should try to enforce (at least internally and during tutorials) using keywords instead of passing arguments as is. We could deal with this in another issue later.

cc @dylanhmorris

dylanhmorris
dylanhmorris approved these changes Jun 4, 2024
View reviewed changes
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

LGTM!

@dylanhmorris dylanhmorris merged commit 394a03d into main Jun 4, 2024
8 checks passed
@dylanhmorris dylanhmorris deleted the dmb_127-all-randomvariables-should-be-recorded-even-if-they-are-deterministic branch June 4, 2024 21:49
damonbayer added a commit that referenced this pull request Jun 5, 2024
@damonbayer
Squashed commit of the following:
9005533 
commit 879c665
Author: George G. Vega Yon <[email protected]>
Date:   Wed Jun 5 11:29:09 2024 -0600

    Weekly Rt with autoregressive difference (#131)

    Co-authored-by: Damon Bayer <[email protected]>
    Co-authored-by: Dylan H. Morris <[email protected]>

commit f0a35ee
Author: George G. Vega Yon <[email protected]>
Date:   Wed Jun 5 10:32:48 2024 -0600

    Renaming datautils to arrayutils (#154)

commit 394a03d
Author: Damon Bayer <[email protected]>
Date:   Tue Jun 4 16:49:10 2024 -0500

    Record deterministic `RandomVariables` by default (#148)

commit a2c1307
Author: Damon Bayer <[email protected]>
Date:   Tue Jun 4 16:02:07 2024 -0500

    read date columns as dates (#153)

commit f11ce38
Author: George G. Vega Yon <[email protected]>
Date:   Mon Jun 3 16:20:08 2024 -0600

    Rt with infection feedback (#123)

    ---------

    Co-authored-by: Dylan H. Morris <[email protected]>
    Co-authored-by: Dylan H. Morris <[email protected]>
    Co-authored-by: Damon Bayer <[email protected]>

commit f9c057a
Author: George G. Vega Yon <[email protected]>
Date:   Fri May 31 13:18:58 2024 -0600

    Refactor transformation module to wrap `numpyro.distributions.transforms` (#140)

    Co-authored-by: Dylan H. Morris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylanhmorris dylanhmorris dylanhmorris approved these changes

@gvegayon gvegayon gvegayon 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.

All RandomVariables should be recorded (even if they are deterministic)
3 participants
@damonbayer @dylanhmorris @gvegayon