DEP: spatial: deprecate complex input to cosine and correlation
#21085
Conversation
spatial.distance.cosine() and correlation()cosine and correlation
There was a problem hiding this comment.
thanks @fancidev, can you make a corresponding post on Discourse?
There was a problem hiding this comment.
Sorry should have read the issue first, complex inputs currently raise an error so do we need to deprecate? I think we could probably go straight to raising a nicer error. wdyt @fancidev @lucascolley
Right now the functions raise a warning or an error depending on the value of the input. There could also be some well-crafted complex input where the functions do not raise either a warning or an error. But the bottom line is, whether a warning or not, the returned value is “arbitrary” in the sense that it’s just whatever the code happens to produce but nothing meaningful. Therefore, In terms of giving a meaningful result, I believe the functions never actually “worked” for complex input. So I’d also be very happy to reject complex input straight away without going through the deprecation phase. |
Thanks for the summary. I would only be in favour of skipping deprecation if, in its current state, the function raises a meaningless error for all complex inputs. I gather from your reply this is not the case so I think we should proceed with deprecation. |
There was a problem hiding this comment.
Sorry one more thing, can you add a node under the relevant parameter docstrings like this
Lines 2696 to 2698 in 8934389
There was a problem hiding this comment.
This definitely looks like a step in the right direction for spatial, based on the associated discussions.
Not sure how consistently we've been adding tests for deprecations, but this seems to fail before/pass after, though is a bit ugly due to all the errors and suppressions involved:
--- a/scipy/spatial/tests/test_distance.py
+++ b/scipy/spatial/tests/test_distance.py
@@ -1639,6 +1639,15 @@ class TestSomeDistanceFunctions:
dist = wcorrelation(x, y)
assert_almost_equal(dist, 1.0 - np.dot(xm, ym) / (norm(xm) * norm(ym)))
+
+ @pytest.mark.filterwarnings('ignore:Casting complex')
+ @pytest.mark.parametrize("func", [correlation, cosine])
+ def test_corr_dep_complex(self, func):
+ arr = np.array([1+2j, 3+4j])
+ with pytest.raises(ValueError):
+ with pytest.deprecated_call(match="Complex `u` and `v` are deprecated"):
+ func(arr, arr)
+
Perhaps more trouble than it is worth, not sure, especially if the suppressions are different on NumPy 1.x vs. 2.x (didn't check).
|
Of course, let's add a note to https://github.com/scipy/scipy/wiki/*-Release-Note-Entries-for-SciPy-1.15.0-(asterisk-makes-this-appear-at-the-top) after merging. |
|
@j-bowhay I added the deprecation message to the doc.
I'm happy to add (or not to add) the tests. Since complex input is never a supported usage nor generates meaningful result, I don't find an urge to test deprecation. On the other hand, there appears no harm to add one, if only for coverage. Please let me know! |
I would prefer the test to be added too |
|
Following @tylerjereddy 's sample code, I added a test case for the I used real-valued complex number in the test case, for two reasons. First, it prevents raising a |
scipy/spatial/distance.py
Outdated
| message = ( | ||
| "Complex `u` and `v` are deprecated and will raise an error in " | ||
| "SciPy 1.17.0.") | ||
| warnings.warn(message, DeprecationWarning, stacklevel=1) |
There was a problem hiding this comment.
| warnings.warn(message, DeprecationWarning, stacklevel=1) | |
| warnings.warn(message, DeprecationWarning, stacklevel=2) |
Done! |
…cipy#21085) * spatial.distance: deprecate complex input to cosine() and correlation(). * Minor refactoring. * Add deprecation message to cosine and correlation doc. * Add test case for DeprecationWarning. * Change stacklevel to 2.
Reference issue
Closes gh-20994.
What does this implement/fix?
If
uorvsupplied tocosine()orcorrelation()is complex, issue aDeprecationWarning.Additional information
Before this PR, these functions do not reject or warn about complex inputs explicitly. However,
ValueError,ComplexWarning, orTypeErrormay occur down the code path depending on the value of the inputs. And even if no error occurs, the returned answer is usually not meaningful. In addition, thecdist/pdistcounterparts of these distance metrics only support real input.To ensure consistency and well-defined behavior, this PR emits a
DeprecationWarningwhen eitheruorvis complex. (wis not checked because aTypeErroris always raised ifwis complex.) The plan is to reject complex input in a future release.See the referenced issue as well as gh-21001 for more details.