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!: new parameter API #63

Merged
merged 12 commits into from
Oct 1, 2023
Merged

Feat!: new parameter API #63

merged 12 commits into from
Oct 1, 2023

Conversation

fjebaker
Copy link
Owner

@fjebaker fjebaker commented Oct 1, 2023

Closes #58

Still todo:

  • Remove all the old binding code
  • Try and discover source of the bug below

Documentation still outstanding but that's for another PR. There's one weird bug in test-fit-multi.jl, where before these changes the very last test fitted

┌ MultiFittingResult:
│    Model: CompositeModel[PowerLaw + PowerLaw]
│    . u     : [17.281, 0.28657, 14.935, 3.0998]
│    . χ²    : 2.8628 
│    Model: CompositeModel[(PowerLaw + PowerLaw) + PowerLaw]
│    . u     : [17.281, 3.7090, 42.203, 0.19553, 39.168, 1.9444]
│    . χ²    : 0.24608 
└ Σχ² = 3.1088

and now

┌ MultiFittingResult:
│    Model: CompositeModel[PowerLaw + PowerLaw]
│    . u     : [17.572, 0.29547, 14.557, 3.1249]
│    . χ²    : 2.9777 
│    Model: CompositeModel[(PowerLaw + PowerLaw) + PowerLaw]
│    . u     : [17.572, 1.0, 40.155, 0.20068, 39.196, 3.2011]
│    . χ²    : 11.864 
└ Σχ² = 14.841

I checked the plots and it is an objectively worse fit. I have no idea why this is the only one that is failing though?

Removed the concept of compile time inference for free/frozen, and all
relevant API. Simplified and reduced the number of `invoke*` functions
to reduce maintenance complexity.

Added various parameter caches to reduce allocations and allow for
update flexibility during fitting.

Most tests passing -- infact only one is broken (commented out for now)
and I don't know why?
@fjebaker
Copy link
Owner Author

fjebaker commented Oct 1, 2023

This is the second time the CI runner has failed to get the data?

@fjebaker
Copy link
Owner Author

fjebaker commented Oct 1, 2023

The fitting bug seems to be that the a power law parameter isn't being fitted when the additive constant is bound... I can't work out why though, since nothing has substantially changed in how the models are invoked...

@fjebaker
Copy link
Owner Author

fjebaker commented Oct 1, 2023

Turning off autodiff gives the right (old) result, so it's some kind of gradient erasure!

@fjebaker
Copy link
Owner Author

fjebaker commented Oct 1, 2023

Between the two model invokes:

# binding :K_1
#         b                     f
Partials(0.1, 3.6, 8.2, 2.0,   0.0, 0.0, 0.0, 0.0, 0.0) # k1
Partials(0.3, 0.0, 0.0, 0.0,   0.0, 0.1, 7.2, 9.1, 7.3) # k2

# we see the gradient does come in though
Partials(0.0, 0.0, 0.0, 0.0,   1.0, 0.0, 0.0, 0.0, 0.0) # parameters

# binding :K_2
#                   b                     f 
Partials(8.2, 2.0, 0.1, 3.6,   0.0, 0.0, 0.0, 0.0, 0.0) # k1 
Partials(0.0, 0.0, 0.3, 0.0,   9.0, 7.3, 0.0, 0.1, 7.2) # k2

There are (4 + 6) 10 model parameters, which it has correctly reduced to 9, but now it has also implicilty managed to pin an additional parameter in the second model.

@fjebaker
Copy link
Owner Author

fjebaker commented Oct 1, 2023

This only happens if the two models being fit have different numbers of model components.

@fjebaker
Copy link
Owner Author

fjebaker commented Oct 1, 2023

Fixed anomalous test condition that was failing 👍

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2023

Codecov Report

Attention: 263 lines in your changes are missing coverage. Please review.

Comparison is base (d8ca4d9) 69.04% compared to head (35d1d7f) 69.09%.
Report is 29 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   69.04%   69.09%   +0.04%     
==========================================
  Files          34       39       +5     
  Lines        1722     1870     +148     
==========================================
+ Hits         1189     1292     +103     
- Misses        533      578      +45     
Files Coverage Δ
src/SpectralFitting.jl 100.00% <ø> (ø)
src/ccall-wrapper.jl 71.91% <ø> (ø)
src/fitting/methods.jl 82.85% <100.00%> (-8.38%) ⬇️
src/fitting/multi-cache.jl 100.00% <100.00%> (ø)
src/fitting/statistics.jl 87.50% <100.00%> (+1.78%) ⬆️
src/generation/function-generation.jl 77.33% <100.00%> (-14.34%) ⬇️
src/julia-models/additive.jl 100.00% <100.00%> (ø)
src/julia-models/multiplicative.jl 100.00% <100.00%> (ø)
src/meta-models/table-models.jl 100.00% <100.00%> (ø)
src/param-cache.jl 100.00% <100.00%> (ø)
... and 26 more

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

@fjebaker fjebaker merged commit daec873 into main Oct 1, 2023
@fjebaker fjebaker deleted the fergus/parameter-api branch October 1, 2023 17:42
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.

Better free/frozen API
2 participants