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

Rename likelihood to stat #2546

Merged
merged 5 commits into from Nov 15, 2019
Merged

Rename likelihood to stat #2546

merged 5 commits into from Nov 15, 2019

Conversation

@cdeil
Copy link
Member

cdeil commented Nov 14, 2019

This pull request fixes #2149, it renames "likelihood", "loglike" terms to "stat".
The idea is that we always use the fit statistic consistently, and for Poisson likelihood it is given by stat = -2 * log(L).
I went with the variant that I prefer, where I use an explicit stat_sum and stat_array for cases where both sum and array are involved. In output results where only the sum is stored, I use stat.

I noticed that the flux point and light curve code is a lot of duplicated copy & paste, I had to do the same edits twice (possibly introducing bugs along the way). Can we do better?

Tests pass, but @adonath - there's probably places I missed?

(I didn't do the dloglike cases yet. Are those TS values? -> have to check)

@cdeil cdeil added the cleanup label Nov 14, 2019
@cdeil cdeil added this to the 0.15 milestone Nov 14, 2019
@cdeil cdeil requested a review from adonath Nov 14, 2019
@cdeil cdeil self-assigned this Nov 14, 2019
@cdeil cdeil added this to In progress in gammapy.modeling via automation Nov 14, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 14, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@e64b351). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2546   +/-   ##
=========================================
  Coverage          ?   91.45%           
=========================================
  Files             ?      144           
  Lines             ?    16296           
  Branches          ?        0           
=========================================
  Hits              ?    14904           
  Misses            ?     1392           
  Partials          ?        0

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 e64b351...674a391. Read the comment docs.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 15, 2019

The remaining task here is to deal with the SED profiles.

From the description here, it sounds to me like loglike, loglike_null and dloglike_scan should not include the factor 2, whereas ts does include it:
https://gamma-astro-data-formats.readthedocs.io/en/latest/spectra/flux_points/index.html#likelihood-columns

I think there was a bug then, because here we put -2 * dloglike in dloglike_scan column?

dloglike_scan = result["likelihood"]
return {"norm_scan": result["values"], "dloglike_scan": dloglike_scan}

So the factor 2 is incorrect, and for the sign I'm not sure.

@adonath - ?

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 15, 2019

@adonath - Ready for final review. Locally make test and make docs-all passes.
Can we merge this in quickly? It touches 20 files and leaving it open blocks other work.

I didn't touch sampling.py. Would prefer to leave this to a separate PR, see #2304 .
It's better to add a test first, and it's ~ 1 day of work.

I checked the plot in tutorials/spectrum_analysis.ipynb - looks ok.

Copy link
Member

adonath left a comment

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

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Nov 15, 2019

@adonath - Thanks for the quick review.
CI fail is unrelated. Merging.

@cdeil cdeil merged commit 6f4f86e into gammapy:master Nov 15, 2019
9 of 10 checks passed
9 of 10 checks passed
greeting
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 24 updated code elements – Tests: passed
Details
gammapy.gammapy Build #20191115.3 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.modeling automation moved this from In progress to Done Nov 15, 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.