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

Integrate background spectrum in MapMaker #1558

Merged

Conversation

@JouvinLea
Copy link

@JouvinLea JouvinLea commented Jul 23, 2018

  • use now the new method integration of background 2D and background3D based on the trapezoidal integration.
  • we saw that f we do a trapezoidal integration compare to the rectangular one, the value of the test implemented for the integration doesn't change that much

Questions that remain:

  • How many integration steps do we put by default? For the moment this is one but it has to be changed. Do we fix it or do we let it as a parameter of the MapMaker method?
  • The test is not easy to understand and thus it is not easy to see that the integration is really working. For the moment there is only a test for background3D but not background 2D in the MapMaker. This is maybe just something we have to remember to improve but I must admit that I will not have time to improve this before leaving in holiday.
@JouvinLea JouvinLea requested review from cdeil and registerrier Jul 23, 2018
@cdeil cdeil added the feature label Jul 23, 2018
@cdeil cdeil added this to To do in gammapy.maps via automation Jul 23, 2018
@cdeil cdeil added this to the 0.8 milestone Jul 23, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 23, 2018

@registerrier (and maybe @AtreyeeS ) - Thoughts?

Where do we want the simple evaluate at log center energy times energy bin width?
And where do we want the more accurate (but also slower / more complex code) log-log-trapz integral?

I think maybe just merge this PR, and n_integration_bins is a good value.
If we start to expose such values from high-level classes like the MapMaker, it gets too complex?
Basically we do something, and then what the user can choose is the energy binning to run the analysis, going finer for more accurate results?

@AtreyeeS
Copy link
Member

@AtreyeeS AtreyeeS commented Jul 24, 2018

I think we should have the option for both evaluate and integrate. My experience with MapMaker is that it is quite time intensive, and making the background takes the maximum time. So, maybe for some simple standard analysis (say, where you just want to extract an image and spectrum but not do anything fancy with it), users can have an option of simple evaluation of log center energy times energy bin width?

@AtreyeeS AtreyeeS mentioned this pull request Jul 24, 2018
@JouvinLea JouvinLea force-pushed the JouvinLea:change_integration_background_MapMaker branch from e340458 to 2365e08 Jul 27, 2018
Copy link
Contributor

@registerrier registerrier left a comment

Thanks.

@registerrier
Copy link
Contributor

@registerrier registerrier commented Jul 27, 2018

Some unrelated fails in the tests. Merging now.

@registerrier registerrier merged commit 8c354f2 into gammapy:master Jul 27, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
gammapy.maps automation moved this from To do to Done Jul 27, 2018
@cdeil cdeil changed the title Change the way of integrating the background rate in MapMaker Integrate background spectrum in MapMaker Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants