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

Performance using Pyirf #83

Merged
merged 18 commits into from Dec 21, 2020
Merged

Conversation

gaia-verna
Copy link
Collaborator

This PR requires #79 and is related to part of #73.
News:

  • a script to calculate performance using pyirf (based on ED analysis)
  • a notebook to display the performance

gaia-verna and others added 5 commits November 7, 2020 20:25
Co-authored-by: adonini <alice.donini@ts.infn.it>
Co-authored-by: adonini <alice.donini@ts.infn.it>
Co-authored-by: adonini <alice.donini@ts.infn.it>
Co-authored-by: adonini <alice.donini@ts.infn.it>
Co-authored-by: adonini <alice.donini@ts.infn.it>
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@HealthyPear HealthyPear added benchmarks input / output new features or issues regarding input and output formats labels Nov 26, 2020
@HealthyPear HealthyPear added this to In progress in Pipeline features and enhancements via automation Nov 26, 2020
@HealthyPear HealthyPear added this to the v0.5.0 milestone Nov 26, 2020
@HealthyPear HealthyPear modified the milestones: v0.5.0, v0.4.0 Dec 1, 2020
@HealthyPear
Copy link
Member

HealthyPear commented Dec 1, 2020

Small spoiler/request before this PR becomes rewieable:

In #76 I have moved all notebooks under docs/contribution/benchmarks.

I would like you to,

  • ignore the PSF notebook, which will be updated anyway,
  • move the new IRFs/sensitivity notebook where the old one will be (under docs/contribution/benchmarks/DL3) but with a different name (add a suffix like "_new" for example so I can later format it / update it before the next release).

EDIT: I just realized that the new notebook has already "_pyirf" as a suffix, so just move it to the new folder.

Copy link
Member

@HealthyPear HealthyPear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so before tackling with the comments I left, make sure you merge master first and resolve the conflicts,

in particular,

  • the config files now are under aux/example_config_files/ and not aux/example_config_files/protopipe
  • the notebook has moved under docs/contribute/benchmarks/DL3, but please retain the _pyirf suffix in the name for now
  • make sure you don't leave data from ASWG in the repository (I removed some, if not all of it in a recent PR)

environment.yml Outdated Show resolved Hide resolved
environment.yml Outdated Show resolved Hide resolved
protopipe/perf/utils.py Outdated Show resolved Hide resolved
Pipeline features and enhancements automation moved this from In progress to Review in progress Dec 11, 2020
@HealthyPear HealthyPear linked an issue Dec 11, 2020 that may be closed by this pull request
Copy link
Member

@HealthyPear HealthyPear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is failing because now that the name of the script changed, the init file of the scripts module needs to be updated

So in protopipe/scripts/__init__.py line 11,

from .make_performance import *

needs to change in,

from .make_performance_EventDisplay import *

protopipe/aux/example_config_files/analysis.yaml Outdated Show resolved Hide resolved
"\n",
"**IMPORTANT**\n",
"\n",
"Soon this will be supersided by new results using [pyirf](https://github.com/cta-observatory/pyirf) and ctaplot metrics."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this section since now we are using pyirf and we mention ctaplot above

protopipe/scripts/make_performance_EventDisplay.py Outdated Show resolved Hide resolved
protopipe/scripts/make_performance_EventDisplay.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #83 (db8d430) into master (f0d73e6) will increase coverage by 6.20%.
The diff coverage is 24.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   30.78%   36.99%   +6.20%     
==========================================
  Files          20       17       -3     
  Lines        2267     1768     -499     
==========================================
- Hits          698      654      -44     
+ Misses       1569     1114     -455     
Impacted Files Coverage Δ
protopipe/scripts/make_performance_EventDisplay.py 21.49% <21.49%> (ø)
protopipe/perf/utils.py 25.49% <35.29%> (+5.49%) ⬆️
protopipe/scripts/__init__.py 100.00% <100.00%> (ø)

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 f0d73e6...db8d430. Read the comment docs.

Copy link
Member

@HealthyPear HealthyPear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 fix missing and 1 small added and then we are good to go!

protopipe/aux/example_config_files/performance.yaml Outdated Show resolved Hide resolved
Pipeline features and enhancements automation moved this from Review in progress to Reviewer approved Dec 21, 2020
@HealthyPear HealthyPear merged commit 70cdb0b into cta-observatory:master Dec 21, 2020
Pipeline features and enhancements automation moved this from Reviewer approved to Done Dec 21, 2020
@HealthyPear HealthyPear added the enhancement New feature or request label Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks enhancement New feature or request input / output new features or issues regarding input and output formats
Development

Successfully merging this pull request may close these issues.

If gammapy gets updated then sensitivity will be not calculated
2 participants