-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
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>
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Small spoiler/request before this PR becomes rewieable:In #76 I have moved all notebooks under I would like you to,
EDIT: I just realized that the new notebook has already "_pyirf" as a suffix, so just move it to the new folder. |
There was a problem hiding this 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 notaux/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)
There was a problem hiding this 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 *
"\n", | ||
"**IMPORTANT**\n", | ||
"\n", | ||
"Soon this will be supersided by new results using [pyirf](/~https://github.com/cta-observatory/pyirf) and ctaplot metrics." |
There was a problem hiding this comment.
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
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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!
This PR requires #79 and is related to part of #73.
News: