search

astropy / specutils

An Astropy coordinated package for astronomical spectroscopy. Maintainers: @rosteen @keflavich @eteq
http://specutils.readthedocs.io/en/latest/
170 stars 127 forks source link

Drop axis option from moment #930

Open pllim opened 2 years ago

pllim commented 2 years ago

@eteq cannot think of a good reason why we need axis option in moment. Maybe @PatrickOgle knows why. Maybe it should just be hardcoded to -1 (spectral axis).

If we decide to drop it, we need to issue a deprecation warning for a period of time before removing it for good. If we do this, we might also make #925 moot because order=2, axis=1 will no longer be a valid use case.

https://github.com/astropy/specutils/blob/1651c007f855c87207e9c8548bd0109baa718927/specutils/analysis/moment.py#L31-L33

rosteen commented 5 months ago

I just talked to @keflavich about this offline - I'm pretty sure that the code to do this was always wrong anyway, since it was always using the spectral axis for the dx and dispersion values even in the case of a spatial axis. I updated in the v2.0dev branch to use pixel values for non-spectral axes - it should probably use the actual physical size of the pixel if available via WCS, but I'm not sure it's worth the effort to check for that case since unclear as to how much value this has anyway, as noted above.