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

moving QuadratureRule over to an equinox.Module and patched up a few … #96

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

cmhamel
Copy link
Collaborator

@cmhamel cmhamel commented Oct 25, 2024

…tests that was using the old QuadratureRule.len method. Now using a len method in QuadratureRule so we can use the python built-in len method.

…tests that was using the old QuadratureRule.len method. Now using a __len__ method in QuadratureRule so we can use the python built-in len method.
@cmhamel
Copy link
Collaborator Author

cmhamel commented Oct 25, 2024

@btalamini You suggested splitting up PR #87 since @ralberd was seeing some performance regressions from that initial stab I took which likely changed too much too fast.

This PR does something simple. Add equinox and jaxtyping as dependencies and converts the QuadratureRule namedtuple implementation to a equinox.Module. jaxtyping just helps document stuff better. It serves no functional purpose other than that.

@ralberd I ran that hole array problem you sent me ~10 times for each implementation and so no real difference.

Also, I left the current constructors in place so it didn't touch every file under the sun. With the equinox class though, you can directly call QuadratureRule(xiguass, wguass).

@cmhamel cmhamel requested review from ralberd and btalamini October 25, 2024 18:23
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.42%. Comparing base (9ea926d) to head (802ba44).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
optimism/Mechanics.py 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   76.40%   76.42%   +0.02%     
==========================================
  Files          61       61              
  Lines        5051     5056       +5     
==========================================
+ Hits         3859     3864       +5     
  Misses       1192     1192              

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

Copy link
Collaborator

@btalamini btalamini left a comment

Choose a reason for hiding this comment

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

This looks really useful! Thanks, Craig!

@cmhamel
Copy link
Collaborator Author

cmhamel commented Oct 28, 2024

@ralberd @btalamini @tupek2

If it's ok with y'all, I can tag the main branch prior to merging this as say "v0.0.1" or "pre-equinox" so if we need to easily revert to pre-equinox stuff it should be fairly straightforward

@btalamini
Copy link
Collaborator

@ralberd @btalamini @tupek2

If it's ok with y'all, I can tag the main branch prior to merging this as say "v0.0.1" or "pre-equinox" so if we need to easily revert to pre-equinox stuff it should be fairly straightforward

That's a good suggestion. Go for it.

@cmhamel cmhamel merged commit bff3a67 into main Oct 28, 2024
4 checks passed
@cmhamel cmhamel deleted the chamel/equinox-quadrature branch October 28, 2024 17:45
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.

3 participants