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

Add DL3+ notebooks #895

Merged
merged 2 commits into from
Feb 3, 2022
Merged

Add DL3+ notebooks #895

merged 2 commits into from
Feb 3, 2022

Conversation

chaimain
Copy link
Contributor

Adding 5 notebooks:

  • MC DL2 to IRF (source-independent, fixed global and energy-dependent gammaness and theta cuts)
  • DL2 to DL3 (source-independent, fixed global and energy-dependent gammaness cuts)
  • DL3 to DL4 (using gammapy v0.19 and saving DL4 as OGIP files)
  • plot LC from DL4 (OGIP files)
  • plot SED from DL4 (OGIP files)

The energy-dependent cuts procedures follows #709

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@chaimain chaimain marked this pull request as draft January 28, 2022 11:03
@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #895 (4db9f1d) into master (71b217e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #895   +/-   ##
=======================================
  Coverage   84.92%   84.92%           
=======================================
  Files          77       77           
  Lines        6170     6170           
=======================================
  Hits         5240     5240           
  Misses        930      930           

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 71b217e...4db9f1d. Read the comment docs.

@chaimain chaimain marked this pull request as ready for review February 1, 2022 16:40
@rlopezcoto
Copy link
Contributor

After v0.9 we should try to make all notebooks work with the current version of lstchain

maxnoe
maxnoe previously requested changes Feb 2, 2022
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

Why are there notebooks for things that are supposed to be done with the tools, e.g. create_dl3_events_file and create_irfs?

lstchain/scripts/lstchain_merge_run_summaries.py Outdated Show resolved Hide resolved
@chaimain
Copy link
Contributor Author

chaimain commented Feb 2, 2022

Why are there notebooks for things that are supposed to be done with the tools, e.g. create_dl3_events_file and create_irfs?

To just show the workflow of the Tools.

@maxnoe
Copy link
Member

maxnoe commented Feb 2, 2022

@chaimain But the notebooks duplicate code from the tools.

@chaimain
Copy link
Contributor Author

chaimain commented Feb 2, 2022

@chaimain But the notebooks duplicate code from the tools.

Yes. I do not understand, how I can explain the workflow of the Tools, without doing so in a jupyter notebook. Can you elaborate more on this?

@maxnoe
Copy link
Member

maxnoe commented Feb 2, 2022

@chaimain You show how you run the tools and explain what they do shortly in prose? Have a look in the inputs/outputs?

We really should not duplicate the code in notebooks that is supposed to be run via command line tools.

People who really want to understand the code need to look at the code, we shouldn't repeat it in a notebook

@chaimain
Copy link
Contributor Author

chaimain commented Feb 2, 2022

@chaimain You show how you run the tools and explain what they do shortly in prose? Have a look in the inputs/outputs?

We really should not duplicate the code in notebooks that is supposed to be run via command line tools.

People who really want to understand the code need to look at the code, we shouldn't repeat it in a notebook

Ok. I understand it now. Thanks!
I will fix the notebooks accordingly.

@chaimain
Copy link
Contributor Author

chaimain commented Feb 2, 2022

@maxnoe should the --help-all section be run in the notebook?

@rlopezcoto rlopezcoto mentioned this pull request Feb 3, 2022
@rlopezcoto
Copy link
Contributor

as commented at the beginning of the discussion, after the new release we should update all notebooks to make them work with the current version of lstchain and some of them like these ones should not contain the information that is better explained elsewhere (as in gammapy notebooks)

@rlopezcoto rlopezcoto dismissed maxnoe’s stale review February 3, 2022 18:11

already discussed and modifications will be implemented in the future

@rlopezcoto rlopezcoto merged commit eeb3f84 into master Feb 3, 2022
@rlopezcoto rlopezcoto deleted the add_dl3_notebooks branch February 3, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants