Skip to content
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

DOC: use np.copysign() instead of np.sign() #19278

Merged
merged 1 commit into from Sep 21, 2023
Merged

Conversation

kalekundert
Copy link
Contributor

The basis vectors returned scipy.linalg.null_space() are guaranteed to be orthonormal, but whether they point in the "positive" or "negative" direction is arbitrary. To remove this source of non-determinism, the 1D documentation example recommends multiplying the basis vector by the sign of its first element.

This seems like good advice, but there's a bug in the way it's implemented. The issue is that np.sign(0) == 0, so if the first element of the basis vector happens to be zero, then the whole vector gets erased. The solution is to use np.copysign(), which doesn't have this behavior (and which even handles -0 correctly).

The basis vectors returned `scipy.linalg.null_space()` are guaranteed to
be orthonormal, but whether they point in the "positive" or "negative"
direction is arbitrary.  To remove this source of non-determinism, the
1D documentation example recommends multiplying the basis vector by the
sign of its first element.

This seems like good advice, but there's a bug in the way it's
implemented.  The issue is that `np.sign(0) == 0`, so if the first
element of the basis vector happens to be zero, then the whole vector
gets erased.  The solution is to use `np.copysign()`, which doesn't
have this behavior (and which even handles -0 correctly).
@mdhaber
Copy link
Contributor

mdhaber commented Sep 21, 2023

Thanks @kalekundert, that sounds like a helpful fix. To confirm that it's working and ensure that it doesn't get broken again, could you add a unit test to scipy/scipy/linalg/tests/test_decomp.py? I'd move test_null_space into a class called TestNullSpace, then add a new test test_gh19278 (or something descriptive) that would fail in main and pass with this PR. At the top of the test, please add a comment that documents what the test is doing (e.g. paraphrase your top post and explain that the test confirms that this is fixed).

@kalekundert
Copy link
Contributor Author

I didn't change any code, so I don't think it's possible to write a test that will fail in main and pass on this branch. I also don't see how a new test case would prevent the documentation from breaking again in the future, since the test wouldn't be connected to the docstring in question. Maybe I could change the documentation example such that it triggers the behavior in question, but I think that would detract from the clarity of the example.

@mdhaber
Copy link
Contributor

mdhaber commented Sep 21, 2023

Oops, I didn't look carefully enough. This is fine as-is. Next time submitting a doc-only change, please include

[skip actions] [skip cirrus]

in the commit message so that only the jobs that build documentation run. The copysign function may be less familiar to users, so it might help to extend the comment about what's going on, but someone can add that later if desired.

@mdhaber mdhaber merged commit 57b15de into scipy:main Sep 21, 2023
22 of 23 checks passed
@kalekundert kalekundert deleted the doc-sign branch September 21, 2023 17:53
@j-bowhay j-bowhay added this to the 1.12.0 milestone Sep 21, 2023
@j-bowhay j-bowhay added scipy.linalg Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org labels Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants