WIP: Further separate statistic from optimization #3786
Conversation
|
I've been working off and on on something similar to this that I think goes even a little further, but I don't think this conflicts in any way with that work, so I think this would be worth having as a start. |
|
@nden – Thanks for picking this up again! The reason you're not getting a correct model amplitude with the cash statistic fit as implemented here is probably because you need to compare observed counts to expected counts per bin, i.e. integrate the continuous model over the bin. An approximate way to do this integration would be to multiply by the bin width. I tried to do this here https://github.com/gammapy/gammapy/pull/127/files#diff-df636a369f23310db5fbfea2896a1654R71 but stopped working on this because I couldn't figure out how astropy.modeling is supposed to work and we started using Sherpa. One concrete suggestion I have here would be to separate the cash statistic function computation like in gammapy.stats.cash, i.e. don't mix it with model evaluation and integration over bins. I have some more thoughts / comments on the API issues that come up here, but I'll make those over at astropy/astropy-api#13 |
|
As I noticed at the Python in Astronomy workshop, some of the docs on this (particularly defining new fitters) is out of date. The work I'm doing is also to make this much easier, so that it requires less fiddling, in general, with calling internal methods in the right places, etc. |
nden
force-pushed
the
further-separation-statistic
branch
from
ef50743 to
7947f17
Compare
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 1 year, 6 months. I plan to close this in a month if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please close this and open an issue instead to revisit in the future. Maintainers may also choose to add If you believe I commented on this issue incorrectly, please report this here. |
|
⏰ Time's up! ⏰ I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
4 participants
Following on discussions during the PIA workshop this PR further separates statistic from optimization. It is possible now to instantiate fitters by passing an optimizer and a statistic function to the
Fitterclass, e.g.Although this eliminates the need I think it may still be good to have some predefined fitters available but I'm not a heavy user of this functionality.
@cdeil I added a cash statistic function following an old example you shared. Testing it with a simple 1D Gaussian shows that the amplitude is consistently overestimated. Could you check that it's implemented correctly.