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

Remove MapDataset cstat likelihood option #2553

Merged
merged 3 commits into from Nov 18, 2019
Merged

Remove MapDataset cstat likelihood option #2553

merged 3 commits into from Nov 18, 2019

Conversation

@cdeil
Copy link
Member

cdeil commented Nov 17, 2019

This is a follow-up to #2546, where we renamed "likelihood" to "stat".

While implementing that PR, I noticed that MapDataset had an option likelihood="cash" which was a string, but then it was set to self.likelihood_type=likelihood in init, and in the class likelihood actually was the function, not the string. I found this confusing.

Now that the function likelihood was renamed to stat it's better, but still, I would prefer to be more explicit and to either have likelihood_type or stat_name or somthing like that as argument for __init__, and then to have the same name for the attribute, or to remove the option completely.

This PR removes the option, which is my preference, because then MapDataset and SpectrumDataset are the same, in SpectrumDataset it was already hard-coded to the cash statistic. I've actually never seen any gamma-ray paper use cstat, I think everyone uses cash. cstat is something that was only used in a few stats papers where the relation to chi2 was explored, showing that they match in the high-stats limit - which we never have in gamma-ray astronomy, there's always low stats at high energies and in small spatial bins, so really IMO offering cstat isn't doing users a favour, and it's not worth the extra effort (documentation, testing). Instead we should offer more powerful ways to customise the likelihood than by a string, e.g. via subclassing and a datasets registry (see #2512 and #2387), for the few % of users that need it (they likely won't want cash, but something else).

@adonath - Thoughts?

@cdeil cdeil added the cleanup label Nov 17, 2019
@cdeil cdeil added this to the 0.15 milestone Nov 17, 2019
@cdeil cdeil added this to In progress in gammapy.cube via automation Nov 17, 2019
Copy link
Member

adonath left a comment

Thanks @cdeil, there is still one test you have to remove as well https://github.com/gammapy/gammapy/blob/master/gammapy/stats/tests/test_fit_statistics.py#L121. I agree that cstat is rarely used and I think it's good to make the fit statistics handling finally uniform.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 18, 2019

Done in 61fda76. Merging.

@cdeil cdeil merged commit 7604f3a into gammapy:master Nov 18, 2019
7 of 10 checks passed
7 of 10 checks passed
greeting
Details
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Analysis: Running – Tests: pending
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.gammapy Build #20191118.2 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
gammapy.cube automation moved this from In progress to Done Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.cube
  
Done
2 participants
You can’t perform that action at this time.