Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
PIG 9 - Event sampling #2136
cdeil left a comment
Some first superficial comments inline.
The basic method proposed here is to sample from binned distributions using inverse CDF, i.e. draw from the npred histogram. That's fine, at least as a first step, Gammapy at the moment is a binned analysis framework and probably other methods aren't within reach at the moment.
But there should be a section (either at the start in an "introduction" or under "alternatives") mentioning that the existing gamma-ray codes that have event samplers do it differently. I don't know much about how the Fermi ST and ctools work, but I do remember that in Gammalib one had to write an
Please start by adding a section with a bit of information, and then we'll try to gather information how others do event sampling - e.g. I'm sure there are HEP papers as well where they describe how they sample events, e.g. UNURAN was added in ROOT for a reason.
Most likely we will for now stick with the method you propose here. Other methods are much more work to implement (requiring a "sample" method on every model), and there's the technical difficulty that with Numpy really only inverse CDF sampling can be done efficiently, because e.g. for rejection sampling (I think) you'd need a Python for loop which is super slow. But these things might come in Gammapy in the future, e.g. with Numba it's possible to implement quite easily.
My point is: this PIG should contain a bit more information why we chose the binned distribution sampling method, and why for now we didn't implement the well-established solution in gamma-ray astronomy, i.e. what Fermi ST and ctools do.
@adonath - FYI: We discussed this in the Gammapy dev call for 5 min today.
My recommendation would be that you finish writing the PIG and post it for review with a 1 week deadline first, or present and discuss it in the dev call next week, and then after that initial feedback circulate it widely for review.
I don't understand what happened in #2204 . The PR description says InverseCDFSampler is added, but I don't think that's what happened in the end and was merged!? Also, looking at the task list in this PIG, it's full of "?", so it's not clear to me what the next PRs will be.
Hi Chris, I'm traveling in this moment, hence sorry if i give you only a quick explanation. In practice, I opened the PR #2204 <#2204> and I actually added the InverseCDF class. However, since it was the first time I was opening a PR, yesterday Axel told me that it's better to split the PR in smaller steps. For this reason, we considered this PR only up to: "This PR introduce a renaming of the gammapy.utils.distributions' into gammapy.utils.random'.
The content of the current `gammapy.utils.random' is moved in the new folder. Then the currently unused GeneralRandomArray class is removed from gammapy.utils.random' ..."
and we didn't included the InverseCDF class yet. I'm going to open, as quick as possible, a new pull requests for the InverseCDF alone. Sorry for the confusion. Also, looking at the task list in this PIG, it's full of "?", so it's not
clear to me what the next PRs will be.
We have to still fix some issues in the PIG. However, after the PR for the InverseCDF class, the next one would have been the pull request #4
"4. Remove the current `GeneralRandom` class and adapt `gammapy.astro.population.simulate' accordingly."
However, about this last point I'll discuss with Axel your suggestion on how to face with it. If you have any question, I'll try to reply as soon as possible. Cheers, F. Il giorno ven 7 giu 2019 alle ore 11:13 Christoph Deil < firstname.lastname@example.org> ha scritto:…
@adonath <https://github.com/adonath> - FYI: We discussed this in the Gammapy dev call for 5 min today. My recommendation would be that you finish writing the PIG and post it for review with a 1 week deadline first, or present and discuss it in the dev call next week, and then after that initial feedback circulate it widely for review. I don't understand what happened in #2204 <#2204> . The PR description says InverseCDFSampler is added, but I don't think that's what happened in the end and was merged!? Also, looking at the task list in this PIG, it's full of "?", so it's not clear to me what the next PRs will be. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2136?email_source=notifications&email_token=AJMDXANIM6MMLIDYAFNWLL3PZIREVA5CNFSM4HL5RIF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXFJJ7Q#issuecomment-499815678>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJMDXAKQL6FKRZO33XEYCILPZIREVANCNFSM4HL5RIFQ> .
registerrier left a comment
Thanks @fabiopintore ! This looks good to me.
lmohrmann left a comment
I'm a bit puzzled by your code example: first, you compute
I also don't quite understand why you need to compute
The rest of the proposal looks OK to me, but this is a major point that should be clarified or changed.
I think for time models the idea is to sample directly, that should be clarified.
For spatial models, it would be possible to sample directly, but would require analytical "sample" methods on each model. Possible, but not what's proposed here.
The most difficult part is to know how many events to sample and the energy distribution. There one has to use
An alternative would be to sample the flux distributions, and then to reject sampled events based on
@lmohrmann - OK to stick with the method to sample
@adonath @fabiopintore - Please clarify the method used (and clarify or remove the code example). If I see correctly, currently the words "exposure" or "effective area" never appear in the PIG -- make it clearer what
I see the advantage of using the existing code to calculate some
@adonath - Can you please add this RST file to the PIG index.rst toctree, so that is shows up in the docs? I usually do that in the PR right before merging and check the HTML. I think this underline is too long and will give a Sphinx warning?