spacetelescope / synphot_refactor

Synthetic photometry using Astropy
http://synphot.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
38 stars 25 forks source link

Separate out units module? #156

Open pllim opened 6 years ago

pllim commented 6 years ago

@robelgeda mentioned that it would be useful for synphot.units to be separated out from this package, so others can take advantage of the flux unit conversion even if they are not interested in synthetic photometry.

In parallel, there are some discussions on Astropy for astropy.units as well following astropy/astropy#7546 (about separating out or making it interoperable with other units packages).

tepickering commented 4 years ago

this might be a worthwhile subject of conversation for the spectroscopy sprint...

kakirastern commented 4 years ago

I think the feature requested is an excellent idea. I actually ran into some related issues with the astropy flux units in specutils which was resolved by the use of synphot units. The concerned spectrum was originally cast in the astropy ST unit. But it was found to be incompatible with specutils when using custom writers and loaders to handle the Spectrum1D object created for the spectrum, and I had to resort to using synphot's equivalent (correct me if I am wrong) flam unit. And the latter seemed to have done the trick.

pllim commented 4 years ago

The unit support here is the same that is supported by HST Exposure Time Calculator, so all HST units are guaranteed to work.

kakirastern commented 4 years ago

I see. Thanks for the info!

pllim commented 4 years ago

Re: spectroscopy sprint -- Please keep me updated on your decisions regarding this matter. Thanks!

dhomeier commented 4 years ago

the spectrum, and I had to resort to using synphot's equivalent (correct me if I am wrong) flam unit. And the latter seemed to have done the trick.

I think that's not quite correct, if I recall correctly flam was a proxy for erg / (s * cm**2 Å); so both are physical_type 'spectral flux density wav', but different units:

>>> u.ST.to('erg/(cm2 s Angstrom)')                                                                                                                                                 
    3.6307805477010036e-09

The situation within specutils is in fact a bit more complex; for reading

  1. The wcs1d-fits loader accepts and correctly parses a BUNIT (i.e. IMAGE-format data) of ST, as it is a standard Astropy unit, but not flam or FLAM.

  2. BINTABLE spectra (e.g. opened with tabular-fits) are read though astropy.table, and this is explicitly parsing the units as u.Unit(col.unit, format='fits', parse_strict='silent'). This is also accepting 'ST', but returning it as u.UnrecognizedUnit. Consequently specutils does not recognise it as flux data, as it does not have the right physical_type and cannot be converted to other flux units.

For specutils this raises the questions if

  1. WCS-based spectra should be parsed similarly strict to tables, and flux units not recognised by the FITS standard be rejected?

  2. Or it should also introduce a workaround for tabular data to recognise and correctly parse valid Astropy units like ST. This might become tricky since the unit parsing is done deep inside table and io.fits, but maybe a related question is if there is a better way for handling units that are outside the FITS standard, but known in the default format space.

kakirastern commented 4 years ago

Yeah, you are right, there is a constant scaler in the conversion between the astropy.units.ST and synphot.units.FLAM. So to be 100% accurate I would need to multiply that factor in to make them truly equivalent. But that the astropy.units.ST is the closest one to doing what I needed.

dhomeier commented 4 years ago

Well the point of using units is of course having them handle this automatically, so you could read your spectrum in as

In [116]: spectrum.flux.unit                                                                                                                                                                  
Out[116]: Unit("ST")

In [117]: spectrum.flux.unit.physical_type                                                                                                                                                    
Out[117]: 'spectral flux density wav'

In [118]: spectrum.flux.to('erg/(cm2 s Angstrom)')                                                                                                                                            
Out[118]: 
<Quantity [1.6202829 , 1.598735  , 1.5542696 , ..., 0.81952107, 0.8158431 ,
           0.        ] erg / (Angstrom cm2 s)>

In [119]: spectrum.flux.to('W/(m2 nm)')                                                                                                                                                       
Out[119]: 
<Quantity [0.01620283, 0.01598735, 0.01554269, ..., 0.00819521, 0.00815843,
           0.        ] W / (m2 nm)>

so my two last points above boil down to

  1. Should we continue to allow wcs1d-fits to do this automatically

  2. Should we try to provide equivalent functionality through tabular-fits or other table loaders

In the context of this issue – would one option for separating it out be to make it available as an additional format option in astropy.units?

pllim commented 4 years ago

I am not sure how to make OBMAG and VEGAMAG generic. One depends on mirror area and the other a Vega spectrum. That's mainly why they are still buried here.

If turns out you don't need the stuff here for the sprint, then maybe we don't need to discuss this right now. 🤷‍♀

kakirastern commented 4 years ago

@pllim I have checked and can confirm that @eteq has indeed left a note in the "Spectroscopic Sprint Week - April 2020" running notes regarding this issue.