Repository navigation
Conversation
The constructors and about a dozen methods tested `if self.transition:` (and `float(transition) if transition else transition`), which is false for 0. With a series indexed from a negative time and the transition at time zero, every indicator was therefore computed over the whole series instead of stopping at the transition. The checks are now `is not None`. Regression tests (fail before, pass after) cover the indicators, detrend, compute_spectrum, and the MultiTimeSeries correlation, covariance and detrend.
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.
transition=0is silently ignored. The constructors and about a dozen methods test truthiness (if self.transition:,float(transition) if transition else transition), and0is falsy, so a series indexed from a negative time with the tipping point at time zero is treated as if no transition had been given: every indicator is computed over the whole series, past the transition.Reproduction on current
main:MultiTimeSeriesbehaves the same way: withtransition=0,compute_corrruns to the last time point (49) as without a transition, whiletransition=30stops at 30.Fix:
is not Noneinstead of truthiness in the two constructors (float(transition) if transition is not None else None), in the 13if self.transition:checks (detrend,compute_var,compute_std,compute_cv,compute_auto,compute_skew,compute_kurt,compute_entropy,compute_dfa,compute_spectrum, anddetrend,compute_covar,compute_corronMultiTimeSeries), and inapply_classifier_inc(not self.transition). Behaviour fortransition=Noneand for non-zero transitions is unchanged;transitionis still stored as afloat.Tests:
tests/test_transition_zero.py(15 tests). 13 of them fail onmainand pass with the fix; the other two pin the unchanged behaviour forNoneand for a positive transition. They covercompute_var,compute_std,compute_cv,compute_skew,compute_kurt,compute_auto,compute_dfa,detrend,compute_spectrum, and theMultiTimeSeriescorrelation, covariance and detrend. One test checks that a series withtransition=0gives the same indicators as the same data shifted by +50 withtransition=50. Full suite: 46 passed, 1 skipped (the TensorFlow one).Not covered by a test:
compute_entropyandapply_classifier_inc(the latter needs a TensorFlow classifier). The change in both is the same one-line condition.CHANGELOG.mdhas an entry under[Unreleased].One thing left alone on purpose:
float(transition)still rejects apd.Timestamp; that is a separate question.