search

open-gamma-ray-astro / gamma-astro-data-formats

Data formats for gamma-ray astronomy
https://gamma-astro-data-formats.readthedocs.io
Creative Commons Attribution 4.0 International
30 stars 27 forks source link

Remove "ph" for "photon" from units #81

Closed cdeil closed 6 years ago

cdeil commented 7 years ago

I would like to propose to consistently remove "ph" for "photon" in unit strings.

I.e. write e.g. "s-1" instead of "ph s-1".

The motivation for this change is that putting "ph" or not results in incompatible units:

>>> import astropy.units as u
>>> u.Unit('ph s-1') == u.Unit('s-1')
False

forcing packages and tools that want to use and check units to standardise on either always having "ph" or not. E.g. in Gammapy we had to add this to make reading and working with the example SEDs given in this spec repo work: https://github.com/gammapy/gammapy/blob/998714db4d799521f9d71d7880abe7190ea995cb/gammapy/spectrum/flux_point.py#L76

Currently some places in the spec don't have "ph" (e.g. the rates in the background models) and the SED example files and description do have it: https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/search?utf8=%E2%9C%93&q=ph&type=

@woodmd (and maybe also @adonath or @joleroi) - Thoughts?

Assuming yes, note that the "ph" is also in the example SED FITS files in this repo like https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/blob/master/source/results/flux_points/flux_points.fits which I can't easily update. Ideally we would switch to example files in the spec that are always generated by scripts (and in many cases could have simple dummy values or zeros) to make this spec more maintainable (even if the example files then don't contain nice data).

gbelanger commented 7 years ago

Yes, indeed! I dropped using ph, photons, or counts a long time ago. The units of event rates are simply events per second, and therefore, just s^-1.

woodmd commented 7 years ago

I'm fine with whatever is the most widely used convention for this. What is used in x-ray astronomy? If we have a consensus about this I can go ahead and take care of updating the SED spec and example files.

gbelanger commented 7 years ago

Some people use ph others not. But I think we should move to drop it. I can't speak for a entire community, though. If you want to poll, you can do a poll. I was going with what seems to me most logical, as @cdeil suggestion, using only s-1.

cdeil commented 7 years ago

It looks like the OGIP document use a mix of "count", "photon" and no unit:

The FITS standard also allows for "count", "ct", "photon", "ph": http://www.aanda.org/articles/aa/full_html/2010/16/aa15362-10/T4.html

The problem is that all of those are considered distinct by unit frameworks like the one in Astropy:

>>> import astropy.units as u
>>> u.Unit('count').to('photon')
UnitConversionError: 'ct' and 'ph' are not convertible
>>> u.Unit('count').to('')
UnitConversionError: 'ct' and '' (dimensionless) are not convertible

In http://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/background/index.html#bkg-2d-format we currently have "s^-1 MeV^-1 sr^-1" as unit for background rates, without "count". My experience is that it's very painful if somewhere one has "count" or "photon", and that it's just not useful to separate counts (possibly including cosmic rays or other background) from photons in code.

As far as I can see, the Fermi catalogs always use "ph" for units of photon fluxes.

So probably, in addition to whatever decision we make here, we should add a section to http://gamma-astro-data-formats.readthedocs.io/en/latest/general/index.html stating something like this?

Units:

The last point would be my preference / recommendation. If it's controversial, of course it could be changed to using "ph" sometimes and sometimes not, and then to state stress that this is one important point codes like Gammapy that are using quantities internally need to handle on I/O (go to something common) to work properly.

cdeil commented 7 years ago

Maybe @jknodlseder or @giacomov as other gamma-ray science tool developers have an opinion here on the general section about units in the spec (see last comment) or specifically on whether to use "photon" or not in unit strings (see description at the top)?

Maybe we're lucky and if there's concensus among this group of people developing codes that handle units results in agreement, and we don't need a larger poll / discussions?

jknodlseder commented 7 years ago

I certainly would not remove "ph" or "photon" which is widely used in OGIP and FITS. It would make the unit string incomplete and ambiguous (for example in contrast to counts s-1)

Le 12 avr. 2017 à 14:39, Christoph Deil notifications@github.com a écrit :

I would like to propose to consistently remove "ph" for "photon" in unit strings.

I.e. write e.g. "s-1" instead of "ph s-1".

The motivation for this change is that putting "ph" or not results in incompatible units:

import astropy.units as u u.Unit('ph s-1') == u.Unit('s-1') False forcing packages and tools that want to use and check units to standardise on either always having "ph" or not. E.g. in Gammapy we had to add this to make reading and working with the example SEDs given in this spec repo work: https://github.com/gammapy/gammapy/blob/998714db4d799521f9d71d7880abe7190ea995cb/gammapy/spectrum/flux_point.py#L76 https://github.com/gammapy/gammapy/blob/998714db4d799521f9d71d7880abe7190ea995cb/gammapy/spectrum/flux_point.py#L76 Currently some places in the spec don't have "ph" (e.g. the rates in the background models) and the SED example files and description do have it: https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/search?utf8=%E2%9C%93&q=ph&type= https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/search?utf8=%E2%9C%93&q=ph&type= @woodmd https://github.com/woodmd (and maybe also @adonath https://github.com/adonath or @joleroi https://github.com/joleroi) - Thoughts?

Assuming yes, note that the "ph" is also in the example SED FITS files in this repo like https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/blob/master/source/results/flux_points/flux_points.fits https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/blob/master/source/results/flux_points/flux_points.fits which I can't easily update. Ideally we would switch to example files in the spec that are always generated by scripts (and in many cases could have simple dummy values or zeros) to make this spec more maintainable (even if the example files then don't contain nice data).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/issues/81, or mute the thread https://github.com/notifications/unsubscribe-auth/AC2oV9_AxmswfqrElbAwEdRETDuKjBEDks5rvMXwgaJpZM4M7Xan.

giacomov commented 7 years ago

I don't have a strong opinion either way.

It seems to me though that the fact that ph and counts are not convertible is a good thing: while a photon is always a count (in a photon-counting experiment), the opposite is not true.

So maybe we should use "counts s-1" for quantities that are not "background subtracted" nor "background free", and use only "photons s-1" for quantities that are (like a photon flux). In this sense keeping them separated (instead of using always just "s-1") might be a good thing.

cdeil commented 7 years ago

@woodmd - Can you please make a decision here (as inventor of the formats and author of the spec)?

There will always be different people with different preferences, and we can't keep accumulating open issues. I'm OK if you reject the proposal I made here. Using "ph" or not is a valid approach.

In any case I'd suggest to add a note on units in the general section as I outlined above. @woodmd - Should I make a separate PR suggesting this addition?

For now I've put a helper function standardise_units in Gammapy to remove the "ph" on read so that we can work with the files: https://github.com/gammapy/gammapy/blob/master/gammapy/utils/units.py I haven't implemented the addition of "ph" on write yet, so at the moment Gammapy is still writing spectra that don't conform to the spec.

To respond to the comments above:

I certainly would not remove "ph" or "photon" which is widely used in OGIP and FITS. It would make the unit string incomplete and ambiguous (for example in contrast to counts s-1)

I don't think dropping the "ph" makes the units "incomplete or ambiguous. We're talking about measured fluxes like cm-2 s-1 here, and the vast majority of gamma-ray papers I've seen puts the unit as cm-2 s-1 in the paper, and not as ph cm-2 s-1. IMO it's very common and well-known in the gamma-ray community.

So maybe we should use "counts s-1" for quantities that are not "background subtracted" nor "background free", and use only "photons s-1" for quantities that are (like a photon flux). In this sense keeping them separated (instead of using always just "s-1") might be a good thing.

Like I said above, my experience is that having "counts" and "photons" as separate units is pretty annoying, because in code you always have to decide which is which and where to convert. We had this in Gammapy for a while. At least in TeV gamma-ray astronomy, very often you don't know if you're dealing with diffuse gamma-ray emission ("photon") or atmospheric cosmic ray background events ("counts") and often you're modeling them both together in an empirical ay without trying to separate them when doing a source analysis.

woodmd commented 7 years ago

I guess I had hoped we could reach some consensus but perhaps this was too optimistic on my part. I tend to agree with @cdeil that there doesn't seem to be a strong advantage to including ph in units and dropping it would simplify the logic for unit validation. To give a concrete example one would often want to compute a photon quantity as the difference between two quantities in counts but according to the logic applied by astropy.units this would be an invalid operation. For any code that ingests these files I suspect inevitably one would end up stripping the ph or ct unit in order to avoid a lot of extra unit handling logic elsewhere. I suspect there are also instances where the correct unit is ambiguous. For instance the LAT always measures some level of diffuse CR contamination so technically you could argue that the diffuse intensities measured by the LAT should be in counts not photons.

As it stands fermipy and gammapy are probably the only two pieces of software using this format for the forseeable future so I think it's better to go with the convention that both package authors agree on. I'll put together a PR in the next few days with the changes to the spec.

giacomov commented 7 years ago

ok, you both have good points, I think dropping them is a sensible solution.

cdeil commented 6 years ago

@woodmd - I think this discussion concluded and we just forgot to close the issue?

I found one last place where the example files contain "ph": https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/blob/83cd4d2c9487aae51956a6d2ef99edd4c5b9e87b/source/spectra/flux_points/binlike.ecsv#L9

Fixed in a commit 0333d79 which I attach here. @woodmd - please review.

cdeil commented 6 years ago

I'm merging this small update to the flux point example files in now. I think it's completely uncontroversial, just a small cleanup. See my last comment.