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

Sensitivity fixes #157

Merged
merged 58 commits into from
Feb 6, 2020
Merged

Conversation

rlopezcoto
Copy link
Contributor

After debugging, together with @moralejo, the sensitivity code, I update here the code with some cleanup. Main changes are:

  • Now we are using crab_hegra everywhere for the sensitivity calculations. There was some inconsistency when transforming from percentage of Crab tu flux units.
  • All the weighting is done in bins of estimated energy, but the energies used for the weighting are the true energies

I also cleaned up the code, but we are still missing some variable clearer names, I'll work on it in the future. Just to report here, during all the debugging we did with @moralejo, we compared all the results at the DL2 level and beyond with those obtained using a Mars-based analysis and everything was equivalent.

Copy link
Member

@vuillaut vuillaut left a comment

Choose a reason for hiding this comment

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

Hi.

Updating the code would be the perfect moment to improve its reading with more explicit names.
I reviewed a couple of things that can be improved (probably not exhaustive).

Also, adding unit tests (especially where they were already bugs! - so they don't happen again) would improve a lot the code.

lstchain/mc/plot_utils.py Outdated Show resolved Hide resolved
lstchain/mc/plot_utils.py Outdated Show resolved Hide resolved
lstchain/mc/plot_utils.py Outdated Show resolved Hide resolved
lstchain/mc/plot_utils.py Outdated Show resolved Hide resolved
lstchain/mc/plot_utils.py Outdated Show resolved Hide resolved
lstchain/mc/sensitivity.py Show resolved Hide resolved
lstchain/mc/sensitivity.py Outdated Show resolved Hide resolved
scripts/lst-sensitivity.py Outdated Show resolved Hide resolved
scripts/lst-sensitivity.py Outdated Show resolved Hide resolved
scripts/lst-sensitivity.py Outdated Show resolved Hide resolved
@rlopezcoto
Copy link
Contributor Author

@vuillaut I implemented all the suggested changes, thanks very much for the review and sorry for the big delay!
I find that at the moment, a complete refactoring of this code should be done, or we can just wait for protopipe to be ready. In any case, with all the real data currently arriving, this is probably not a priority.

@vuillaut
Copy link
Member

@vuillaut I implemented all the suggested changes, thanks very much for the review and sorry for the big delay!
I find that at the moment, a complete refactoring of this code should be done, or we can just wait for protopipe to be ready. In any case, with all the real data currently arriving, this is probably not a priority.

Hey @rlopezcoto, thank you very much.
The final goal was to have a dedicated tool for IRF production independent of other libraries (ctapipe, lstchain, etc...). In the meantime, this code will be very useful.

@rlopezcoto
Copy link
Contributor Author

@vuillaut can we merge this one?

Copy link
Member

@vuillaut vuillaut left a comment

Choose a reason for hiding this comment

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

Hi.
there are still a few changes required for functions to work.
Could you add some unit tests, please?
-> maybe not a unit test for each function but at least a high-level one once the dl2 file has been created by the chain to make sure the functions work in principle and show how the user should make the sensitivity computation?

lstchain/scripts/lstchain_mc_sensitivity.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_mc_sensitivity.py Show resolved Hide resolved
lstchain/scripts/lstchain_mc_sensitivity.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_mc_sensitivity.py Outdated Show resolved Hide resolved
lstchain/scripts/lstchain_mc_sensitivity.py Outdated Show resolved Hide resolved
lstchain/mc/plot_utils.py Outdated Show resolved Hide resolved
lstchain/mc/plot_utils.py Outdated Show resolved Hide resolved
'eff_gamma', 'eff_hadron',
'nevents_g', 'nevents_p'])

units = [E.unit, E.unit,"", t.unit,"", "",
units = [energy.unit, energy.unit,"", t.unit,"", "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

t is not defined now, "theta2_bins"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

@rlopezcoto rlopezcoto left a comment

Choose a reason for hiding this comment

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

Thanks @SeiyaNozaki for spotting the new problems in the PR!

@rlopezcoto
Copy link
Contributor Author

Thanks to @vuillaut for the changes as well! Not sure what is breaking the tests, but we have two options:

  1. Include a dl1 proton file
  2. Deactivate the test
    In any of the two cases, I would like to move on with this PR for the people to start using it and also correct subsequent errors that I'm sure will appear.

@vuillaut
Copy link
Member

I'm looking into the unit test issue.

@rlopezcoto
Copy link
Contributor Author

Hi, I used the creation of a fake proton file as described by @vuillaut in this PR (sorry, but I could not merge it because there were conflicts and had to solve the issues quickly).
I corrected all the issues with the units and also the problems because we were using diffuse gammas for the tests to evaluate the sensitivity. Now all checks are passing, so let's move forward with this PR.

@vuillaut
Copy link
Member

vuillaut commented Feb 6, 2020

Thank you @rlopezcoto !

@rlopezcoto rlopezcoto merged commit 5181fa6 into cta-observatory:master Feb 6, 2020
@rlopezcoto rlopezcoto deleted the sensitivity_fixes branch February 10, 2020 15:14
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