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

simulate_3d notebook is broken #2040

Closed
registerrier opened this issue Feb 20, 2019 · 6 comments
Closed

simulate_3d notebook is broken #2040

registerrier opened this issue Feb 20, 2019 · 6 comments
Assignees
Labels
Milestone

Comments

@registerrier
Copy link
Contributor

registerrier commented Feb 20, 2019

After the introduction of FixedPointingInfo and the modification of make_map_background_irf to use FOV coordinates in PR #2033 , the notebook simulate_3d.ipynb is broken.
This is due to the fact that make_map_background_irf now takes a FixedPointingInfo rather than a coordinate as input.
Two possible solutions:

  • we create a FixedPointingInfo with a dict containing all required information in the notebook. This basically mean that we need an event file besides the IRFs to read it from. Or we need to build the dict programmatically.
  • we adapt make_map_background_irf to be able to deal with a simple SkyCoord for the pointing, in which case no transformation to AltAz is performed. This is not really nice but has the advantage of being backward compatible.
    Opinions @adonath @AtreyeeS @watsonjj ?
@registerrier registerrier added this to the 0.11 milestone Feb 20, 2019
@adonath
Copy link
Member

adonath commented Feb 20, 2019

@registerrier I committed a preliminary fix in e697a3e, reading the pointing info from one of the 1dc event files.

I think the make_map_background_irf is not really a function to be used by users, but rather an implementation detail. So I think it's fine to do the background-incompatible change and stick with the FixedPointingInfo...

@AtreyeeS
Copy link
Member

@adonath As a user, I would prefer to be able to simulate maps from scratch, specially for positions with no previous observations. In such a case, reading pointing info from previous observations is not really a solution. If you don't want to adapt make_map_background_irf, then I think the notebook should clearly demonstrate how to create a FixedPointingInfo

@adonath
Copy link
Member

adonath commented Feb 20, 2019

I see you point @AtreyeeS, but I think in practice it turns out to be difficult, because for the FixedPointingInfo you have to define parameters such as location and observation time and you have to make sure that your target is actually in the FoV at this time. So you basically enter the business of observation planning...which is OK, but it extends the scope of the simulate_3d.ipynb a lot.

So I'm not 100% sure what to do here. To me it seems the simulate_3d.ipynb currently describes an example that you would not do in practice. Either you start from an existing observation and you have all the information available or you start by defining a pointing strategy for your observations and create the data from this. The question is in which direction we want to go with the simulate_3d.ipynb notebook...

@AtreyeeS
Copy link
Member

Erm, I see the technical issue here, but it is really unpleasant. I will not want to "plan" an entire observation is most cases. Perhaps we can have a dummy pointing_creator() which takes a SkyCoord and creates a pointing for this specific case? I don't know either.
But what we are having now does not seem good. A user should be able to simulate maps without worrying about the pointing strategies.

@adonath adonath self-assigned this Feb 20, 2019
@registerrier
Copy link
Contributor Author

registerrier commented Feb 20, 2019

Indeed, there are different use cases for MC simulation besides the full event sampler. Typically:

  • simulate what a certain model would yield in a given dataset to check fit results for instance.
  • create a fake observation result if you want to predict the sensitivity

The later should not require a PointingInfo because it might not be relevant at all. e.g. you simulate a very long observation that cannot be performed in reality etc. I think this feature is very frequently used at the moment in gammapy.

So I am not sure we should completely get rid of it.
In itself make_map_background_irf is an implementation detail indeed, but it is the only option to create background maps for this task so far.

@adonath
Copy link
Member

adonath commented Mar 3, 2019

In #2052, @registerrier added back the option to evaluate the background by giving a pointing position on the sky, now both is possible.

@adonath adonath closed this as completed Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants