Skip to content

Handle new PhysicalType class in spectrum_from_column_mapping #833

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Handle new PhysicalType class in spectrum_from_column_mapping #833

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f838862
BUG: update `spectrum_from_column_mapping` to handle astropy 4.3 `Phy…
dhomeier Jun 23, 2021
b8c7a25
Fix quotes in cm_type test
dhomeier Jun 23, 2021
60bb7fb
Use string comparisons where supported
dhomeier Jun 23, 2021
fcf624b
Revert log() formatting; comment tests [ci skip]
dhomeier Jun 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions specutils/io/default_loaders/tabular_fits.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,12 @@ def tabular_fits_writer(spectrum, file_name, hdu=1, update_header=False, **kwarg
wunit = u.Unit(kwargs.pop('wunit', spectrum.spectral_axis.unit))
disp = spectrum.spectral_axis.to(wunit, equivalencies=u.spectral())

# Mapping of spectral_axis types to header TTYPE1
dispname = wunit.physical_type
# Mapping of spectral_axis types to header TTYPE1 (no "torque/work" types!)
dispname = str(wunit.physical_type)
if dispname == "length":
dispname = "wavelength"
elif "energy" in dispname:
dispname = "energy"

# Add flux array and unit
ftype = kwargs.pop('ftype', spectrum.flux.dtype)
Expand Down
11 changes: 5 additions & 6 deletions specutils/io/parsing_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,17 @@ def spectrum_from_column_mapping(table, column_mapping, wcs=None):
kwarg_val = u.Quantity(table[col_name], tab_unit)

# Attempt to convert the table unit to the user-defined unit.
log.debug("Attempting auto-convert of table unit '%s' to "
"user-provided unit '%s'.", tab_unit, cm_unit)
log.debug(f"Attempting auto-convert of table unit {tab_unit} to "
f"user-provided unit {cm_unit}.")
Copy link
Contributor

@nmearl nmearl Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logging library in python is optimized to use %s formatting (ref, ref), which is why you'll find it throughout the codebase. I realize it's not strictly enforced, so this is probably a fine change (easier to parse > optimization), but just for future reference.

Copy link
Member

@pllim pllim Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... but does this line run so much that it warrants optimization?

In [14]: %timeit log.debug(f"'{x.unit}'")
3.59 µs ± 176 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [15]: %timeit log.debug("'%s'" % x.unit)
1.49 µs ± 5.85 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

Copy link
Contributor Author

@dhomeier dhomeier Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually log.debug("'%s'", x.unit) which is probably using .format() internally:

In [8]: %timeit log.debug("'%s'", x.unit)
526 ns ± 8.8 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [9]: %timeit log.debug("'%s'".format(x.unit))
611 ns ± 11.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

Still, the speedup in this context is very modest compared to the time the actual code takes

In [10]: %timeit x.to(u.nm)
10.6 µs ± 199 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [11]: %timeit x.to(u.Hz, equivalencies=u.spectral())
64.5 µs ± 2.22 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

I find the original version easy enough to parse; just needs to be known since f-strings are otherwise strongly endorsed now in astropy core.

Copy link
Contributor

@nmearl nmearl Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhomeier is exactly right, the logging package is built to be passed arguments which are evaluated as-needed. In the case of f-strings, interpolation happens before logging visibility criteria is evaluated, which can lead to cases where string errors are raised, nothing gets logged, and the program exits because of the automatic evaluation of __str__. In %s-style, interpolation happens within the logging package and only as needed. It will therefore be caught as a logging-related issue, an error raised as log message, and the code will continue executing.

Additionally, log aggregation (i.e. the sentry package), works because the %s string template is the same between similar logging statements. Obviously, we don't make prolific use of this, nor do we experience string-related log-avoiding bugs, but there is a reason that python did not switch the package to using {} or f-style string interpolation with python 3.

Copy link
Member

@pllim pllim Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, just because astropy uses it doesn't mean specutils has to. I don't think astropy uses much logging internally.

Though you probably want to document this somewhere in your dev docs, if not already.

Copy link
Contributor Author

@dhomeier dhomeier Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I also took this solely as a recommendation, but still think it a good idea to synchronise the styles of coordinated packages unless there are more important considerations against it like here.

Copy link
Contributor Author

@dhomeier dhomeier Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though you probably want to document this somewhere in your dev docs, if not already.

I don't think the dev docs here include anything on code style; might be best to just link to https://docs.astropy.org/en/latest/development/codeguide.html
But I believe the performance considerations could be of value there, too, even if logging is currently used very little (found some direct usage of logging in the FITS scripts but hardly any of its custom AstropyLogger) – maybe just adding a note to the recommendation on f-strings.

Copy link
Member

@pllim pllim Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't bring AstropyLogger into this... 😆 There was a separate discussion, which resulted in doc update clarifying that the logger is for astropy internal use only.

If you want to put the doc upstream in astropy dev docs, I'll have to loop in @embray here since he's working on dev docs revamp over there. They would have a better idea whether this particular doc should go.

Copy link
Contributor Author

@dhomeier dhomeier Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no intention to switch to AstropyLogger here, was just looking where logging is used inside astropy, if at all.
If it isn't, there's obviously not much need to mention its own formatting interface. But it probably could not hurt either...


if not isinstance(cm_unit, u.Unit):
cm_unit = u.Unit(cm_unit)
if cm_unit.physical_type in ('length', 'frequency'):
if cm_unit.physical_type in ('length', 'frequency', 'energy'):
# Spectral axis column information
kwarg_val = kwarg_val.to(cm_unit, equivalencies=u.spectral())
elif 'spectral flux' in cm_unit.physical_type:
elif 'spectral flux' in str(cm_unit.physical_type):
# Flux/error column information
kwarg_val = kwarg_val.to(
cm_unit, equivalencies=u.spectral_density(1 * u.AA))
kwarg_val = kwarg_val.to(cm_unit, equivalencies=u.spectral_density(1 * u.AA))
elif tab_unit:
# The user has provided no unit in the column mapping, so we
# use the unit as defined in the table object.
Expand Down
8 changes: 4 additions & 4 deletions specutils/tests/test_loaders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,14 +522,14 @@ def test_tabular_fits_writer(tmpdir, spectral_axis):
spectrum.write(tmpfile, format='tabular-fits')
spectrum.write(tmpfile, format='tabular-fits', overwrite=True)

cmap = {spectral_axis: ('spectral_axis', wlu[spectral_axis]),
cmap = {spectral_axis: ('spectral_axis', 'micron'),
Copy link
Contributor

@nmearl nmearl Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this changed? It seems to defeat the purpose of the parameterized test.

Copy link
Contributor Author

@dhomeier dhomeier Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wlu[spectral_axis] is simply the original unit disp was created with, so for spec.spectral_axis the original test did not cover custom unit conversion by the mapping (and therefore did not find the bug of not identifying energy as a physical_type correctly).
I guess this might be moved to a separate check, but I don't see any problems with testing spectral_axis and flux units in the same run.

'flux': ('flux', 'erg / (s cm**2 AA)'),
'uncertainty': ('uncertainty', None)}

# Read it back again and check against the original
spec = Spectrum1D.read(tmpfile, format='tabular-fits', column_mapping=cmap)
assert spec.flux.unit == u.Unit('erg / (s cm**2 AA)')
assert spec.spectral_axis.unit == spectrum.spectral_axis.unit
assert spec.spectral_axis.unit == u.um
assert quantity_allclose(spec.spectral_axis, spectrum.spectral_axis)
assert quantity_allclose(spec.flux, spectrum.flux)
assert quantity_allclose(spec.uncertainty.quantity,
Expand Down Expand Up @@ -566,13 +566,13 @@ def test_tabular_fits_multid(tmpdir, ndim, spectral_axis):
spectrum.uncertainty.quantity)

# Test again, using `column_mapping` to convert to different flux unit
cmap = {spectral_axis: ('spectral_axis', wlu[spectral_axis]),
cmap = {spectral_axis: ('spectral_axis', 'THz'),
'flux': ('flux', 'erg / (s cm**2 AA)'),
'uncertainty': ('uncertainty', None)}

spec = Spectrum1D.read(tmpfile, format='tabular-fits', column_mapping=cmap)
assert spec.flux.unit == u.Unit('erg / (s cm**2 AA)')
assert spec.spectral_axis.unit == spectrum.spectral_axis.unit
assert spec.spectral_axis.unit == u.THz
assert quantity_allclose(spec.spectral_axis, spectrum.spectral_axis)
assert quantity_allclose(spec.flux, spectrum.flux)
assert quantity_allclose(spec.uncertainty.quantity,
Expand Down