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

Raise exception when spectral and flux axes have different lengths #508

Merged
merged 6 commits into from
Sep 10, 2019

Conversation

brechmos-stsci
Copy link

This will check the spectral_axis and flux axis lengths to make sure they are the same and will raise an exception if that is not the case.

Fixes #326

specutils/tests/test_spectrum1d.py Outdated Show resolved Hide resolved
specutils/tests/test_spectrum1d.py Outdated Show resolved Hide resolved
specutils/spectra/spectrum1d.py Outdated Show resolved Hide resolved
@dhomeier
Copy link
Contributor

dhomeier commented Sep 9, 2019

Just a note on the docs/diagnostics: the error message now states "must be the same (or uncertainty must be a singleton", but a length-1 or scalar uncertainty is actually rejected.

@@ -112,6 +118,12 @@ def __init__(self, flux=None, spectral_axis=None, wcs=None,
data=flux.value if isinstance(flux, u.Quantity) else flux,
wcs=wcs, **kwargs)

if hasattr(self, 'uncertainty') and self.uncertainty is not None:
if not flux.shape == self.uncertainty.array.shape:
raise ValueError('Flux axis ({}) and uncertainty ({}) shapes must be the same (or uncertainty must be a singleton)'.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError('Flux axis ({}) and uncertainty ({}) shapes must be the same (or uncertainty must be a singleton)'.format(
raise ValueError('Flux axis ({}) and uncertainty ({}) shapes must be the same.'.format(

Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

One minor suggestion but otherwise this looks good to me.

Note: I'm approving to indicate "I like it overall, and don't need to see it again unless someone wants more feedback from me", but the inline comment is meant to mean "don't merge without the comment either being accepting or intentionally rejected" (i.e., if there's a good reason not to be doing this then that's fine).

specutils/spectra/spectrum1d.py Outdated Show resolved Hide resolved
Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>
@brechmos-stsci brechmos-stsci merged commit bc47905 into astropy:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spectrum1D constructor should check whether flux and uncertainty dimensions match
5 participants