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

Combine the post DL3 notebooks, to solve few minor issues #955

Merged
merged 7 commits into from
May 2, 2022

Conversation

chaimain
Copy link
Contributor

The LC was not produced appropriately, by providing any custom energy range, as gammapy required the flux points edges to be the same as the "energy" axis (used for the events) provided while creating the SpectrumDataset.

So, to keep the process uniform, I combine the 3 notebooks, back into a single notebook, where the energy range for LC estimation depends on the energy edges of the energy_axis. Also, energy_axis and energy_fit_edges have the same edges.

Other changes are -

  • Option of applying custom safe energy masks, in blocks 19 and 20, instead of using SafeMaskMaker
  • Small fix on displaying (or saving) the various plots of each dataset (counts+excess, exposure and energy dispersion) in block 25, to avoid issues when having large datasets.
  • OGIP files are still written, but not used in this example to produce LC and SED. They can be used easily, but for further simplicity, I avoid doing so here.
  • After performing the Fit function, using the updated spectral model parameters, for estimating the LC from the dataset
  • Storing the Optimization results along with the final model into a file, in blocks 44 and 56
  • In block 57, I save the flux points tables of SED and LC data as HDUs in a fits file, and in the final blocks, show how one can use the fits file to reproduce the LC and SED. This fits file is not based on GADF standard, but just a temporary usage, where we can compare the results of different analyses (or sources if need be).

I request analyzers to review and check if there is a need for some more modification or information in this general example notebook.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

notebooks/post_DL3_analysis.ipynb Show resolved Hide resolved
notebooks/post_DL3_analysis.ipynb Show resolved Hide resolved
notebooks/post_DL3_analysis.ipynb Show resolved Hide resolved
notebooks/post_DL3_analysis.ipynb Show resolved Hide resolved
notebooks/post_DL3_analysis.ipynb Show resolved Hide resolved
notebooks/post_DL3_analysis.ipynb Show resolved Hide resolved
notebooks/post_DL3_analysis.ipynb Show resolved Hide resolved
@gabemery
Copy link
Collaborator

Very good notebook but I still had a couple of comments. Some of them need to be solved before merging.

It seems like your build is failing due to "lstchain_longterm_dl1_check" which is weird.

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #955 (7dd6700) into master (b1f83fa) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #955      +/-   ##
==========================================
+ Coverage   85.46%   85.49%   +0.03%     
==========================================
  Files          78       78              
  Lines        6461     6461              
==========================================
+ Hits         5522     5524       +2     
+ Misses        939      937       -2     
Impacted Files Coverage Δ
lstchain/reco/r0_to_dl1.py 93.67% <0.00%> (+0.63%) ⬆️

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 b1f83fa...7dd6700. Read the comment docs.

@maxnoe
Copy link
Member

maxnoe commented Apr 4, 2022

Shouldn't the original notebooks also be removed?

@chaimain
Copy link
Contributor Author

chaimain commented Apr 4, 2022

Shouldn't the original notebooks also be removed?

Ah Sorry, I thought I had done so.

@chaimain chaimain requested review from gabemery and maxnoe April 4, 2022 13:55
@chaimain chaimain requested a review from gabemery April 25, 2022 09:07
Copy link
Collaborator

@gabemery gabemery left a comment

Choose a reason for hiding this comment

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

Latest changes look good. I didn't go over previously reviewed code again. I think you can merge.

@moralejo moralejo merged commit 33d5511 into master May 2, 2022
@moralejo moralejo deleted the fix_dl3_notebook branch May 2, 2022 16:42
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.

4 participants