-
Notifications
You must be signed in to change notification settings - Fork 107
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
Allow a way to circumvent monotonic wavelength requirement in Spectrum1D #1708
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
85effbc
Add sensible error message
tbowers7 a9cc26a
Update specutils version to accomodate this PR
tbowers7 1b6f0ac
monotonic check
kbwestfall b02e23c
need to iteratively remove points
kbwestfall 33a1520
changes
kbwestfall 4379d29
test
kbwestfall d1c5b30
Merge branch 'develop' into update_specutils_loaders
tbowers7 37b5078
Merge remote-tracking branch 'upstream/specutil_wave' into update_spe…
tbowers7 795230d
Update release notes
tbowers7 ac053fe
Merge pull request #1709 from pypeit/update_specutils_loaders
kbwestfall e00758a
PR comments
kbwestfall 0c7e651
Merge branch 'develop' into specutil_wave
kbwestfall File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,11 +14,13 @@ | |
from pypeit import specobjs | ||
from pypeit.specutils import Spectrum1D, SpectrumList | ||
from pypeit.tests import tstutils | ||
from pypeit.pypmsgs import PypeItError | ||
|
||
import pytest | ||
specutils_required = pytest.mark.skipif(Spectrum1D is None or SpectrumList is None, | ||
reason='specutils not installed') | ||
|
||
|
||
@specutils_required | ||
def test_onespec_io(): | ||
rng = np.random.default_rng() | ||
|
@@ -134,3 +136,32 @@ def test_spec1d_io(): | |
|
||
ofile.unlink() | ||
|
||
|
||
@specutils_required | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
def test_onespec_monotonic(): | ||
rng = np.random.default_rng(999) | ||
grid_wave = np.linspace(3500,10000,1000) | ||
wave = grid_wave + (10*rng.uniform(size=grid_wave.size) - 1) | ||
flux = np.ones(1000, dtype=float) | ||
# TODO: PYP_SPEC is required if we want to be able to read the file! | ||
spec = onespec.OneSpec(wave, grid_wave, flux, PYP_SPEC='shane_kast_blue') | ||
ofile = Path(tstutils.data_path('tmp.fits')).resolve() | ||
spec.to_file(str(ofile), overwrite=True) | ||
|
||
with pytest.raises(PypeItError): | ||
# Should fault because the wavelength vector is not monotonically | ||
# increasing | ||
_spec = Spectrum1D.read(ofile) | ||
|
||
# This will be fine because the grid *is* monotonically increasing | ||
_spec = Spectrum1D.read(ofile, grid=True) | ||
|
||
# This should be fine because reader will remove non-monotonically | ||
# increasing wavelength measurements. | ||
__spec = Spectrum1D.read(ofile, strict=False) | ||
|
||
assert _spec.shape[0] > __spec.shape[0], 'Strict should remove data' | ||
|
||
ofile.unlink() | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make it an optional argument, then?
ivar=None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if this function were moved to somewhere in
core
to be used outside of this loader, then making this argument optional is fine. Otherwise, this is just a helper function for use within this module and it doesn't matter sinceivar
in some form will be passed in all the time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although @rcooke-ast 's suggestion is perfectly reasonable, I agree with @tbowers7 : this is really just a helper function that isn't intended to be used outside this module. To signal this is a "private" function, I added a leading underscore to the function name.