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

Maximum likelihood for Rainbow #407

Merged
merged 8 commits into from
Sep 10, 2024
Merged

Conversation

karpov-sv
Copy link
Contributor

Switch Rainbow to maximum likelihood based cost function. It allows to correctly implement upper limits (using Tobit model for censored data) and additional barrier terms that would prevent the parameters from wandering too close to their limits (as then Hessian estimation would be wrong, giving extremely large errors, due to internal non-linear transformations of the parameters inside iminuit).
Improve initial values estimation and make them a bit more error-prone.
Optimize some computations inside model function (zeros_like is much slower than plain zeros)
Also, slightly improve code readability by generalizing parameter unscaling. It allows to correctly return the covariance matrix.

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.41%. Comparing base (c504d8c) to head (6250e66).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
- Coverage   82.45%   82.41%   -0.04%     
==========================================
  Files           9        9              
  Lines        2570     2565       -5     
==========================================
- Hits         2119     2114       -5     
  Misses        451      451              

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

Copy link

codspeed-hq bot commented Sep 5, 2024

CodSpeed Performance Report

Merging #407 will not alter performance

Comparing karpov-sv:rainbow_ml (6250e66) with master (c504d8c)

Summary

✅ 99 untouched benchmarks

@hombit hombit requested review from hombit and erusseil September 5, 2024 14:54
Copy link
Member

@hombit hombit left a comment

Choose a reason for hiding this comment

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

Thank you, @karpov-sv, it looks really nice. I'd like @erusseil to review it before the merge.

@karpov-sv, do you think it may go to a patch (e.g. 0.9.4) release?

@karpov-sv
Copy link
Contributor Author

@karpov-sv, do you think it may go to a patch (e.g. 0.9.4) release?

Yes, it would be ok, as the code is declared experimental anyway. The changes from the point of view of "primary" API should be minimal (just a bit more stable).

Copy link
Contributor

@erusseil erusseil left a comment

Choose a reason for hiding this comment

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

Thanks a lot Sergey for the work, it looks promising! I made some tests and everything appears very robust to me.

@hombit hombit enabled auto-merge September 10, 2024 12:52
@hombit hombit disabled auto-merge September 10, 2024 15:15
@hombit hombit merged commit b40aa28 into light-curve:master Sep 10, 2024
15 of 16 checks passed
@hombit
Copy link
Member

hombit commented Sep 11, 2024

@karpov-sv @erusseil v0.9.4 has been released, and you both are now listed as co-authors of the package (as I should do earlier). Thank you!

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