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

Refactor convolve scanner factory functions #161

Conversation

dylanhmorris
Copy link
Collaborator

@dylanhmorris dylanhmorris commented Jun 5, 2024

Ready for review.

This PR:

Out of scope

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.78%. Comparing base (48b7471) to head (b5178a6).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #161   +/-   ##
=======================================
  Coverage   94.77%   94.78%           
=======================================
  Files          39       39           
  Lines         842      843    +1     
=======================================
+ Hits          798      799    +1     
  Misses         44       44           
Flag Coverage Δ
unittests 94.78% <100.00%> (+<0.01%) ⬆️

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 dylanhmorris marked this pull request as ready for review June 6, 2024 23:02
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! Just a couple of comments.

model/src/pyrenew/convolve.py Outdated Show resolved Hide resolved
model/src/pyrenew/convolve.py Outdated Show resolved Hide resolved
model/src/pyrenew/convolve.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@damonbayer damonbayer left a comment

Choose a reason for hiding this comment

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

Should we check the size/shape/type of inputs?

model/src/pyrenew/convolve.py Outdated Show resolved Hide resolved
model/src/pyrenew/convolve.py Outdated Show resolved Hide resolved
@dylanhmorris
Copy link
Collaborator Author

Should we check the size/shape/type of inputs?

@damonbayer are you talking about checking the inputs to the factories or about having the functions they produce/return perform input checks?

@damonbayer
Copy link
Collaborator

@damonbayer are you talking about checking the inputs to the factories or about having the functions they produce/return perform input checks?

Checking inputs to factories so we can provide useful errors.

@dylanhmorris
Copy link
Collaborator Author

dylanhmorris commented Jun 10, 2024

Things that occur to me:

  1. Check that the double scanner tuples are of exactly length two. As written, it will error if they are less.
  2. Check that the dists are ArrayLike and the transforms are callable.
  3. Check that the pair of dists are of the same length (in the future, we might want to have the double scanner just use history subsets of the length of the longer array, and automagically sub-subset when taking the dot product with the shorter array, but that feels a bit implicit.

Stricter stuff that I'm more reluctant to implement:

  1. Could try to check that the transforms are shape-preserving for jax arrays, but that's a bit tricky.
  2. Could force the arrays that go into the dot products to be flat.

Anything else you had in mind @damonbayer?

@damonbayer
Copy link
Collaborator

@dylanhmorris Sounds good to me.

@damonbayer damonbayer added this to the L Sprint milestone Jun 10, 2024
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.

All looks good. Test pass and the website is rendered correctly.

@damonbayer damonbayer merged commit 81c49f1 into main Jun 13, 2024
8 checks passed
@damonbayer damonbayer deleted the 147-add-transforms-to-new_convolve_scanner-make-identity-transforms-default-for-new_double_scanner branch June 13, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add transforms to new_convolve_scanner, make identity transforms default for new_double_scanner
3 participants