-
Notifications
You must be signed in to change notification settings - Fork 205
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
Implement power law estimation with 2 disconnect intervals #3381
base: RELEASE_next_minor
Are you sure you want to change the base?
Implement power law estimation with 2 disconnect intervals #3381
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## RELEASE_next_minor #3381 +/- ##
======================================================
- Coverage 80.66% 80.61% -0.06%
======================================================
Files 146 146
Lines 21941 21927 -14
Branches 5171 5175 +4
======================================================
- Hits 17699 17676 -23
- Misses 3026 3031 +5
- Partials 1216 1220 +4 ☔ View full report in Codecov by Sentry. |
Is it possible to do more than 2 interval? |
I would also be interested to have two intervals, e.g. for background fitting of a linear function. |
Using the two-area method the function can only take 2 intervals. Eventually one could use the same function to estimate the background on more intervals and then find a way to consolidate the results. Another approach could be linearizing the problem and using linear least squares to estimate the parameters. The we can have as many regions as we like. I have just tested this approach and the estimation is less robust to noise than the two-area method, and takes 2x more time for a single interval. However, it might be competitive timewise for more than 2 intervals. @jlaehne, since the polynomial component uses I think that in this PR we could stick to the two-area method with 2 intervals. We can consider adding more intervals and/or adding multi-interval estimation to other components in a separate pull request. However, we should agree on the API for multiple intervals. Any suggestions? |
ok, then we should implement an API that can support more than two intervals. The options could be:
Option 2 needs a bit more work to go through the deprecation but is possibly more convenient to use, as it avoid having to define start and end of a given interval in two different arrays. Another consideration is that this would consistent with the way the |
Here are some alternatives:
|
Yes, that would be convenient to support ROIs. |
I guess for using it with the background removal tool, ROIs would also make sense? But I have not looked into the inner workings of the function. |
Description of the change
So far,
hs.model.compones1D.PowerLaw.estimate_parameters
could estimate the parameters of the power law using the two-area method on an interval defined by thex1
andx2
. Here we implement defining 2 disconnected intervals for estimation by adding thex3
andx4
keyword arguments.Progress of the PR
upcoming_changes
folder (seeupcoming_changes/README.rst
),readthedocs
doc build of this PR (link in github checks)Minimal example of the bug fix or the new feature