Fix precision loss for identical subsequences - #1206
Conversation
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1206 |
seanlaw
left a comment
seanlaw
left a comment
There was a problem hiding this comment.
@iAnjaneySingh I've left a comment for you to address and consider
| pearson = min(1.0, pearson) | ||
|
|
||
| if pearson > 1.0 - 1e-8 and np.array_equal( | ||
| T_A[uint64_i : uint64_i + uint64_m], |
There was a problem hiding this comment.
The _compute_diagonal function is the most important function in our entire package and needs to be highly performant (i.e., without bloat or unneeded code).
Adding np.array_equal is bad because:
- it would dramatically slow down the computation, say, for a long time series with mostly zero values
- This does not take into account time series with identical subsequences that are only different by a constant vertical shift
Additionally, what's missing is that you should clearly demonstrate that the originally failing test is passing after this fix is applied. That is the most important criteria (in addition to the regular test suite passing)
There was a problem hiding this comment.
Thanks for the detailed feedback, Sean. That makes sense, especially since _compute_diagonal is performance-critical. I’ll remove the np.array_equal check and look into a more efficient numerical fix that also handles subsequences that differ by a constant shift. I will also make sure to clearly show the original failing test passing after the fix, along with the full test suite results.
There was a problem hiding this comment.
@iAnjaneySingh Another approach that we had discussed before was adding a (optional) post-processing step. So, after the matrix profile is computed, could we take the subsequence pairs that have:
- a distance/pearson value below a certain threshold
- (nearly) identical standard deviations
- select a handful of data points (e.g., 4 data points) from each pair of subsequences and see if their pairwise differences are the same
- Maybe some other fast criteria that leverages numbers that we've already computed
If so, then we recompute the dot product for these two subsequences.
2 participants
Description
Fixes #1160.
When two identical subsequences are compared, the rolling covariance
recurrence can accumulate a small floating-point error. This can produce
a Pearson correlation slightly below 1.0 and therefore a small non-zero
distance for exactly identical subsequences.
This change detects cases where the computed Pearson correlation is
extremely close to 1.0 and the two raw subsequences are exactly equal,
then sets the Pearson correlation to 1.0.
Validation
rolling covariance recurrence.
tests/test_stump.py: 29 passed.Pull Request Checklist
Below is a simple checklist but please do not hesitate to ask for assistance!
black(i.e.,python -m pip install blackorconda install -c conda-forge black)flake8(i.e.,python -m pip install flake8orconda install -c conda-forge flake8)pytest-cov(i.e.,python -m pip install pytest-covorconda install -c conda-forge pytest-cov)black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./in the root stumpy directoryflake8 --extend-exclude=.venv ./in the root stumpy directory./setup.sh dev && ./test.shin the root stumpy directory and ensured that all tests are passing locallyPlease do not commit any code to avoid/circumvent a failing test and, instead, engage in a discussion (below) to determine the best course of action.
Only request a review after the checklist above is fully completed!