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 FluxPointsDataset chi2assym option #2645

Merged
merged 2 commits into from Dec 1, 2019
Merged

Conversation

@cdeil
Copy link
Member

cdeil commented Dec 1, 2019

This PR removes the chi2assym from FluxPointsDataset.

It wasn't covered by a test, and not well documented (with an example and in the stats docs, explaining it's relation to normal chi2 and likelihood profiles).

My main argument why we shouldn't offer is is that is isn't standard or useful really. At https://cxc.harvard.edu/sherpa/statistics/ there's many chi2 variants, but the asymmetric one isn't one of them. I don't recall having seen the asymmetric chi2 used in papers at all. And it's not a good technique, if the likelihood is asymmetric, then the likelihood profile method which we should add soon is much better.

This is also a follow-up to #2546, removing the last case of confusing of "likelihood" vs "stat" vs "likelihood_type" names in the Gammapy codebase.

@cdeil cdeil added the cleanup label Dec 1, 2019
@cdeil cdeil added this to the 0.15 milestone Dec 1, 2019
@cdeil cdeil requested a review from adonath Dec 1, 2019
@cdeil cdeil self-assigned this Dec 1, 2019
@cdeil cdeil added this to To do in gammapy.spectrum via automation Dec 1, 2019
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Dec 1, 2019

It was used here in the SED fitting tutorial: https://travis-ci.org/gammapy/gammapy/jobs/619286087#L1804

I noticed that drop_ul here wasn't workig here:
https://docs.gammapy.org/0.14/notebooks/sed_fitting_gammacat_fermi.html#Load-spectral-points
It kept the UL columns, because for 3FHL and 3GL the nan values were in dnde_errn, but the drop_ul method only checked the dnde column.
This is the only place where we call drop_ul, and it was buggy.

@adonath - Given that it's difficult to define exactly how drop_ul should work, i.e. what columns it should check, how about we simply delete that method?
For now, I'll just fix this case manually, without calling drop_ul.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 1, 2019

Codecov Report

Merging #2645 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2645      +/-   ##
==========================================
+ Coverage   91.55%   91.61%   +0.05%     
==========================================
  Files         141      141              
  Lines       15883    15867      -16     
==========================================
- Hits        14542    14536       -6     
+ Misses       1341     1331      -10
Impacted Files Coverage Δ
gammapy/spectrum/flux_point.py 92.46% <100%> (+1.43%) ⬆️
gammapy/spectrum/make.py 97.01% <0%> (-0.05%) ⬇️
gammapy/analysis/core.py 97.57% <0%> (-0.03%) ⬇️
gammapy/spectrum/dataset.py 95.45% <0%> (ø) ⬆️
gammapy/maps/wcsnd.py 96.31% <0%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f223a2...1ca8283. Read the comment docs.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Dec 1, 2019

Fixed in 1ca8283 .

@adonath - Are you OK with this removal?
(not sure if I added this asym feature or you or if you care about it)

@adonath
adonath approved these changes Dec 1, 2019
Copy link
Member

adonath left a comment

Thanks @cdeil! No further comments from my side...

@cdeil cdeil merged commit 78380f6 into gammapy:master Dec 1, 2019
12 checks passed
12 checks passed
greeting
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
codecov/patch 100% of diff hit (target 91.55%)
Details
codecov/project 91.61% (+0.05%) compared to 4f223a2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy #20191201.5 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.spectrum automation moved this from To do to Done Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.