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

Separate out units module? #156

Open
pllim opened this issue Jun 25, 2018 · 10 comments
Open

Separate out units module? #156

pllim opened this issue Jun 25, 2018 · 10 comments

Comments

@pllim
Copy link
Contributor

pllim commented Jun 25, 2018

@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
Copy link

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

@kakirastern
Copy link

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
Copy link
Contributor Author

pllim commented Apr 6, 2020

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

@kakirastern
Copy link

I see. Thanks for the info!

@pllim
Copy link
Contributor Author

pllim commented Apr 6, 2020

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

@dhomeier
Copy link

dhomeier commented Apr 7, 2020

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
Copy link

kakirastern commented Apr 7, 2020

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
Copy link

dhomeier commented Apr 7, 2020

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
Copy link
Contributor Author

pllim commented Apr 7, 2020

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
Copy link

@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.

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

4 participants