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

Change from live to on time in background computation #1861

Merged
merged 3 commits into from Oct 5, 2018

Conversation

@registerrier
Copy link
Contributor

@registerrier registerrier commented Oct 5, 2018

This PR changes the calls to make_map_background_irfto take ontime i.e. (TSTOP-TSTART) rather than livetime (i.e. ontime corrected by dead time fraction) to follow the specifications presented in:
https://gamma-astro-data-formats.readthedocs.io/en/stable/irfs/full_enclosure/bkg/index.html#notes

docstring is changed as well as the call in MapMakerObs. Tests are updated accordingly.

@registerrier registerrier added this to the 0.9 milestone Oct 5, 2018
@registerrier registerrier self-assigned this Oct 5, 2018
@registerrier registerrier added this to To do in gammapy.maps via automation Oct 5, 2018
@registerrier registerrier requested a review from AtreyeeS Oct 5, 2018
@cdeil
cdeil approved these changes Oct 5, 2018
Copy link
Member

@cdeil cdeil left a comment

@registerrier - Thanks!

You use "ontime" here, but on the events and obs container the property is called "observation_time_duration".

We should probably add a section below http://docs.gammapy.org/0.8/development/howto.html#coordinate-and-axis-names where we define names to be used consistently throughout Gammapy, i.e. in the obs classes, events classes, and functions like this one. My suggestion would be to just use "observation_time" for this case, because it's a little more intuitive than "ontime"? But either way is OK for me, I just don't like the mix. @registerrier - Could you open a separate PR with a short section and suggestions for names? (I don't think we need to stick to the current names on the obs and events classes, we're allowed to make changes and clean and better classes once now.)
And then implementing the renames for consistency can be done one PR at a time in the coming weeks, e.g. @dcfidalgo could do it for the obs classes; trying to discuss and decide the names, and editing many places in the code wouldn't be pleasant.

@cdeil cdeil merged commit c9838d4 into gammapy:master Oct 5, 2018
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 2 new issues – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
gammapy.maps automation moved this from To do to Done Oct 5, 2018
@cdeil cdeil changed the title Change livetime for background Change from live to on time in background computation Nov 23, 2018
@registerrier registerrier deleted the registerrier:change_livetime_for_background branch Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants