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

Start refactoring pyIRF #36

Merged
merged 110 commits into from Sep 27, 2020
Merged

Start refactoring pyIRF #36

merged 110 commits into from Sep 27, 2020

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Sep 18, 2020

Implemented in the first commit:

  • reading of EventDisplay DL2 fits files
  • Calculating event weights
  • Functions for binning events
  • Li&Ma Significance

Definition of the internal data format, for now astropy.table.QTables
with several columns expected for DL2 event lists.

Co-authored-by: Michele Peresano michele.peresano@cea.fr
Co-authored-by: Lukas Nickel lukas.nickel@tu-dortmund.de

More to come

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #36 into master will increase coverage by 71.39%.
The diff coverage is 72.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #36       +/-   ##
===========================================
+ Coverage    0.72%   72.12%   +71.39%     
===========================================
  Files           9       29       +20     
  Lines         685      696       +11     
===========================================
+ Hits            5      502      +497     
+ Misses        680      194      -486     
Impacted Files Coverage Δ
pyirf/cut_optimization.py 0.00% <0.00%> (ø)
pyirf/sensitivity.py 0.00% <0.00%> (ø)
pyirf/spectral.py 0.00% <0.00%> (ø)
pyirf/statistics.py 0.00% <0.00%> (ø)
pyirf/io/eventdisplay.py 40.00% <40.00%> (ø)
pyirf/utils.py 61.90% <61.90%> (ø)
pyirf/io/gadf.py 84.37% <84.37%> (ø)
pyirf/simulations.py 91.66% <91.66%> (ø)
pyirf/benchmarks/energy_bias_resolution.py 93.10% <93.10%> (ø)
pyirf/binning.py 94.73% <94.73%> (ø)
... and 45 more

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 0bf3f40...7c802a5. Read the comment docs.

maxnoe and others added 10 commits September 21, 2020 11:13
Implemented in this commit:

* reading of EventDisplay DL2 fits files
* Calculating event weights
* Functions for binning events
* Li&Ma Significance

Definition of the internal data format, for now astropy.table.QTables
with several columns expected for DL2 event lists.

Co-authored-by: Michele Peresano <michele.peresano@cea.fr>
Co-authored-by: Lukas Nickel <lukas.nickel@tu-dortmund.de>
Co-authored-by: Michele Peresano <michele.peresano@cea.fr>
Co-authored-by: Lukas Nickel <lukas.nickel@tu-dortmund.de>
pyirf/statistics.py Outdated Show resolved Hide resolved
The hEMC contains events weighted to a spectrum of -2.5,
not the number of simulated CORSIKA showers.

We now compute this information from the guesstimated number
of runs (number of unique obs ids from the events table) and the
run header information.
@maxnoe
Copy link
Member Author

maxnoe commented Sep 22, 2020

Short update here:

With the current version of the script in exaples/calculate_eventdisplay_irfs.py, we calculate a sensitivity that looks correctly calculated but which is different from the event display one due to different cut optimization, especially theta cuts are very different as what EventDisplay calculates does not seem to be a simple, bin-wise 68% containment.

pyirf_sensitivity

At the moment, we do:

  • Calculate theta cut as 68 % containment radius in very fine reco energy bins with a soft gamma hadron cut applied
  • Optimize the gh cut in the bins of the sensitivity

@HealthyPear
Copy link
Member

At the moment, we do:

* Calculate theta cut as 68 % containment radius in very fine reco energy bins with a soft gamma hadron cut applied

* Optimize the gh cut in the bins of the sensitivity

As far as I understood we are then only missing the 3rd step, which is to re-calculate the bin-wise 68% containment angular cut after the g/h bin-wise optimization

Next release automation moved this from Review in progress to Reviewer approved Sep 27, 2020
HealthyPear
HealthyPear previously approved these changes Sep 27, 2020
@HealthyPear
Copy link
Member

we could add the codacy badga again though, for me it was working

@maxnoe
Copy link
Member Author

maxnoe commented Sep 27, 2020

If it works, great. Maybe codacy was just down for a while when I tried.

@HealthyPear
Copy link
Member

HealthyPear commented Sep 27, 2020

If it works, great. Maybe codacy was just down for a while when I tried.

It should: the badge was pointing to this url- though, it's the master, the PR should be automatic I think

I will re-add it by copy-pasting the original RST command from their website just to be sure there wasn't a typo

what about the GitHub pages build? what are the equivalent RST links for image and target?
I couldn' find a specific github pages deployment badge, and using the one of travis just doubled the information.
I added a specific link for the docs, even though I think it's still easier for people to use the much easier to see link in the about page (I personally never used a readme to go to the docs...)

I think we are good to merge now

Next release automation moved this from Reviewer approved to Review in progress Sep 27, 2020
@HealthyPear HealthyPear self-requested a review September 27, 2020 11:49
HealthyPear
HealthyPear previously approved these changes Sep 27, 2020
Next release automation moved this from Review in progress to Reviewer approved Sep 27, 2020
@HealthyPear
Copy link
Member

HealthyPear commented Sep 27, 2020

I just realized that we should probably add cython to extras_require["tests"], otherwise gammapy makes the installation crash for that section.

Modifying setup.py in this way worked for me. Can you confirm?

@HealthyPear HealthyPear dismissed their stale review September 27, 2020 12:20

bug in installation

Next release automation moved this from Reviewer approved to Review in progress Sep 27, 2020
Next release automation moved this from Review in progress to Reviewer approved Sep 27, 2020
HealthyPear
HealthyPear previously approved these changes Sep 27, 2020
Next release automation moved this from Reviewer approved to Review in progress Sep 27, 2020
Next release automation moved this from Review in progress to Reviewer approved Sep 27, 2020
@maxnoe maxnoe merged commit a3cec78 into master Sep 27, 2020
Next release automation moved this from Reviewer approved to Done Sep 27, 2020
@maxnoe maxnoe deleted the rewrite branch September 27, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Next release
  
Done
5 participants