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

Triple-Gauss PSF format #21

Open
2 of 7 tasks
cdeil opened this issue Jan 13, 2016 · 5 comments
Open
2 of 7 tasks

Triple-Gauss PSF format #21

cdeil opened this issue Jan 13, 2016 · 5 comments
Milestone

Comments

@cdeil
Copy link
Member

cdeil commented Jan 13, 2016

I've added a formula for the triple-Gauss PSF:
http://gamma-astro-data-formats.readthedocs.org/en/latest/irfs/psf/psf_3gauss/index.html
as well as some background info on radial PSFs:
http://gamma-astro-data-formats.readthedocs.org/en/latest/irfs/psf/index.html#point-spread-function

I think some discussion / checks are needed:

  • Is the formula correct? I took it from the HAP code that fits the PSF and added the 1/pi factor to go from dP/dr^2 (which is what HAP fits) to dP/d\Omega (which is what Fermi and OGIP use as PSF value).
  • Is there a more sane / stable parametrisation for multi-Gauss (look what PSFEx do) that we want to adopt?
  • Can the presentation / explanations be improved?
  • @mimayer - Does the PA exporter use the same parametrisation?
    ==> there is no 3-gauss exporter in PA.
  • @JouvinLea - Do you use multi-Gauss in HAP-FR exporters? Same parametrisation?
    ===> This is work in progress, HAP-FR can put whatever we decide here.
  • I'd like to check the Gammapy code
  • @mimayer - Is gammalib.GCTAPsf2D consistent with this definition?
@cdeil cdeil self-assigned this Jan 13, 2016
@cdeil cdeil added this to the 0.1 milestone Jan 13, 2016
@cdeil
Copy link
Member Author

cdeil commented Feb 2, 2016

@JouvinLea commented via email:

why do you have S and not a normalization factor 1/(1pi_(s1_2 + A2 * S2_2 + A3_S3*2))? I mean the PSF has to be normalize no?

I agree, having a normalised PSF is nicer.
The only reason to maybe not do it is backwards-compatibility with existing exporters / data / tools.

Thoughts?

@cdeil
Copy link
Member Author

cdeil commented Feb 2, 2016

@jknodlseder - Could you please comment on the following questions concerning the triple-Gauss PSF format?

  • Do you think changing to a parametrisation that is automatically normalised to integrate to 1 (like the King PSF) is a good idea? This would mean dropping SCALE as a parameter / column and putting the right normalisation factor in front of the format.
  • Would this change still be acceptable in Gammalib? Maybe via a "version=2" header keyword?

I think for the coming months / years we need some scheme to do backwards-incompatible changes to DL3 formats anyways to avoid being stuck with early mistakes, and we could use this as a test case on how to do this?
But of course this only makes sense if it's agreed that the change is good, i.e. the new PSF parameterisation is an improvement.

@jknodlseder
Copy link
Collaborator

The SCALE parameter is in fact not used by GammaLib as the GCTAPsf2D class assures internally the normalisation. So in principle the SCALE parameter can be dropped. For the moment, the order of the parameter blocks in the FITS file are hardwired in GCTAPsf2D which is anyways a bad idea. Enhancing GCTAPsf2D so that the required parameter blocks are dynamically extracted from the column names would handle the two formats transparently. So we would not need to use the version header keyword for that (and to increment the number).

You may create a change request asking for an automatic detection of the parameter blocks based on column names. Note that this implies that the column names will then be fixed.

We also need to change GCTAResponseIrf::load_psf which for the moment checks the presence of columns and infers from that whether to use a Gaussian or a King PSF. We just need to remove the table.contains("SCALE") condition.

Le 2 févr. 2016 à 12:37, Christoph Deil a écrit :

@jknodlseder - Could you please comment on the following questions concerning the triple-Gauss PSF format?

Do you think changing to a parametrisation that is automatically normalised to integrate to 1 (like the King PSF) is a good idea? This would mean dropping SCALE as a parameter / column and putting the right normalisation factor in front of the format.
Would this change still be acceptable in Gammalib? Maybe via a "version=2" header keyword?
I think for the coming months / years we need some scheme to do backwards-incompatible changes to DL3 formats anyways to avoid being stuck with early mistakes, and we could use this as a test case on how to do this?
But of course this only makes sense if it's agreed that the change is good, i.e. the new PSF parameterisation is an improvement.


Reply to this email directly or view it on GitHub.

@cdeil
Copy link
Member Author

cdeil commented Feb 3, 2016

@jknodlseder - Thank you for the quick reply!

Change request for Gammalib is here:
https://cta-redmine.irap.omp.eu/issues/1656

We also need to change GCTAResponseIrf::load_psf which for the moment checks the presence of columns and infers from that whether to use a Gaussian or a King PSF. We just need to remove the table.contains("SCALE") condition.

I would much prefer to have declarative scheme. E.g. HDUCLASS and maybe a version as e.g. here:
http://gamma-astro-data-formats.readthedocs.org/en/latest/irfs/background/index.html#bkg-3d-format

Having to maintain code that pokes and guesses formats in Gammalib / Gammapy for the coming years is painful and not really needed if we tell data producers via these specs what to put, right?
(of course, short-term we have to keep doing this to keep the tools working with existing files...)

@jknodlseder
Copy link
Collaborator

I fully agree that a declarative scheme should be implemented.

As we had so far no conventions, we simply put the emphasize on a system that "works", but now that the conventions start to emerge, we should of course fully support them.

Le 3 févr. 2016 à 11:02, Christoph Deil a écrit :

@jknodlseder - Thank you for the quick reply!

Change request for Gammalib is here:
https://cta-redmine.irap.omp.eu/issues/1656

We also need to change GCTAResponseIrf::load_psf which for the moment checks the presence of columns and infers from that whether to use a Gaussian or a King PSF. We just need to remove the table.contains("SCALE") condition.

I would much prefer to have declarative scheme. E.g. HDUCLASS and maybe a version as e.g. here:
http://gamma-astro-data-formats.readthedocs.org/en/latest/irfs/background/index.html#bkg-3d-format

Having to maintain code that pokes and guesses formats in Gammalib / Gammapy for the coming years is painful and not really needed if we tell data producers via these specs what to put, right?
(of course, short-term we have to keep doing this to keep the tools working with existing files...)


Reply to this email directly or view it on GitHub.

@cdeil cdeil modified the milestones: 0.1, 0.2 Apr 5, 2016
@cdeil cdeil removed the enhancement label Apr 17, 2016
@cdeil cdeil modified the milestones: 0.2, 0.3 Jan 18, 2018
@cdeil cdeil removed their assignment Aug 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants