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

feat: add caching of constant sub expressions in SympyModels #247

Merged
merged 5 commits into from
Mar 26, 2021

Conversation

spflueger
Copy link
Member

Closes #100

@spflueger
Copy link
Member Author

spflueger commented Mar 24, 2021

I'm keeping it like this for now. Not the best user interface, but I suspect we have more issues regarding the Model interface and the gradient as well. So I think this Model interface just does not cut it...

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #247 (737df5d) into master (8485e82) will increase coverage by 1.46%.
The diff coverage is 80.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
+ Coverage   79.71%   81.17%   +1.46%     
==========================================
  Files          13       13              
  Lines         641      712      +71     
  Branches       97      119      +22     
==========================================
+ Hits          511      578      +67     
- Misses         93       96       +3     
- Partials       37       38       +1     
Flag Coverage Δ
unittests 81.17% <80.90%> (+1.46%) ⬆️

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

Impacted Files Coverage Δ
src/tensorwaves/estimator.py 80.00% <72.72%> (-5.37%) ⬇️
src/tensorwaves/model.py 77.33% <81.81%> (+17.09%) ⬆️

Copy link
Member

@redeboer redeboer left a comment

Choose a reason for hiding this comment

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

Good job!
I left a few comments. It's a bit obscure why the macos job keeps failing now, looking into it. This is a problem that sporadically happened before, so this has to be tackled anyway. (Still, it's strange that this PR makes the macos job more unstable.)

src/tensorwaves/model.py Show resolved Hide resolved
src/tensorwaves/model.py Show resolved Hide resolved
@redeboer redeboer force-pushed the issue100 branch 3 times, most recently from cc6b313 to 4c63396 Compare March 26, 2021 10:24
@redeboer
Copy link
Member

A series of PRs related to the callbacks is coming up that might fix the CI problem seen here.

@spflueger spflueger merged commit bbd2c0a into master Mar 26, 2021
@spflueger spflueger deleted the issue100 branch March 26, 2021 17:49
@redeboer redeboer mentioned this pull request May 13, 2021
@redeboer redeboer modified the milestones: 0.2.4, 0.2.2 May 27, 2021
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.

Caching of partial calculations
2 participants