Conversation
…en uneven
welch, mtm and periodogram refused unevenly-spaced input from inside
utils/spectral.py with a terse message ('For the Welch method, data should
be evenly spaced') that named neither the caller's method nor any fix.
Check at the Series.spectral() call site instead, the way Series.wavelet()
already does for cwt, and name the options: interp()/bin()/gkernel(), or
lomb_scargle/wwz which handle uneven sampling. The utils-layer guards stay
as the backstop for callers who reach specutils directly - the same split
wavelet() uses.
Fixes LinkedEarth#698
This branch has not been deployed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #698.
Problem
welch,mtmandperiodogramrefuse unevenly-spaced input from deep insidepyleoclim/utils/spectral.py:The message names neither the method the user actually called nor anything they can do about it.
Series.wavelet()already does better forcwt, raising at the call site with the fix spelled out.Change
A single guard at the top of
Series.spectral(), mirroringSeries.wavelet():lomb_scargleandwwzare deliberately excluded — both are built for uneven sampling.cwtis included so all four evenly-spaced-only methods report the same way; it previously surfaced fromutils/wavelet.py.On "move" vs. "add"
The issue says move the check up. I raised at the call site but left the
utils/spectral.pyguards in place, because removing them would silently drop validation for anyone callingspecutils.welch()/mtm()/periodogram()directly — a public entry point. This is also exactly the splitwavelet()uses today: an actionable raise inSeries.wavelet()plus the low-level check still sitting atutils/wavelet.py:2531. Happy to strip the utils guards if you'd rather the check live in only one place.Testing
Two new parametrized tests in
TestUISeriesSpectral:test_spectral_uneven_raises_with_guidance— for each ofwelch/mtm/periodogram/cwt, asserts the error names the method, mentionsinterp, and points atlomb_scargle.test_spectral_uneven_allowed_methods— assertslomb_scargleandwwzstill run on the same uneven series, so the guard can't over-reach.Confirmed they guard the change: with the
series.pyhunk stashed, all fouruneven_raisescases fail.pytest pyleoclim/tests/test_core_Series.py— 257 passed.