Skip to content

Conversation

h-vetinari
Copy link
Member

Follow-up to #20454

@github-actions github-actions bot added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure DX Everything related to making the experience of working on SciPy more pleasant Meson Items related to the introduction of Meson as the new build system for SciPy labels Apr 14, 2024
@h-vetinari h-vetinari removed the request for review from rgommers April 14, 2024 23:53
@h-vetinari h-vetinari removed the Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org label Apr 14, 2024
@h-vetinari h-vetinari requested a review from rgommers April 14, 2024 23:54
@h-vetinari
Copy link
Member Author

Fun! Switching from gcc 8 to 9 produces a test failure (looks like switched signs somewhere):

=================================== FAILURES ===================================
________________________________ test_svds[COO] ________________________________
[gw0] linux -- Python 3.10.14 /opt/hostedtoolcache/Python/3.10.14/x64/bin/python

matrices = (array([[3.55222419, 2.37499147, 2.6603491 , 3.11783787, 2.74582631,
        1.68472138, 3.81263702, 2.64125678, 2.096...y([0.612149  , 0.91819808, 0.62573667, 0.70599757, 0.14983372,
       0.74606341, 0.83100699, 0.63372577, 0.43830988]))

    def test_svds(matrices):
        A_dense, A_sparse, v0 = matrices
    
        u0, s0, vt0 = splin.svds(A_dense, k=2, v0=v0)
        u, s, vt = splin.svds(A_sparse, k=2, v0=v0)
    
        assert_allclose(s, s0)
>       assert_allclose(u, u0)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-07, atol=0
E       
E       Mismatched elements: 9 / 18 (50%)
E       Max absolute difference: 0.93086219
E       Max relative difference: 2.
E        x: array([[-0.415099, -0.371749],
E              [ 0.465431, -0.322283],
E              [ 0.428654, -0.343568],...
E        y: array([[ 0.415099, -0.371749],
E              [-0.465431, -0.322283],
E              [-0.428654, -0.343568],...

@rgommers
Copy link
Member

Those tests are wrong. They should be using the same logic as in scipy/sparse/linalg/_eigen/tests/test_svds.py for comparing sparse and dense results:

    # Check that scipy.sparse.linalg.svds ~ scipy.linalg.svd
    if check_svd:
        u2, s2, vh2 = sorted_svd(A, k, which)
        assert_allclose(np.abs(u), np.abs(u2), atol=atol, rtol=rtol)
        assert_allclose(s, s2, atol=atol, rtol=rtol)
        assert_allclose(np.abs(vh), np.abs(vh2), atol=atol, rtol=rtol)

@lucascolley lucascolley added the maintenance Items related to regular maintenance tasks label Apr 15, 2024
@lucascolley lucascolley changed the title Increase minimum required compiler versions to GCC 9.1 and Clang 14.0; adapt CI CI/MAINT: Increase minimum required compiler versions to GCC 9.1 and Clang 14.0; adapt CI Apr 15, 2024
@lucascolley lucascolley removed the DX Everything related to making the experience of working on SciPy more pleasant label Apr 15, 2024
@h-vetinari
Copy link
Member Author

They should be using the same logic as in scipy/sparse/linalg/_eigen/tests/test_svds.py for comparing sparse and dense results:

Handled in #20484

@rgommers rgommers added this to the 1.14.0 milestone Apr 16, 2024
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's give this a go. Thanks @h-vetinari

@rgommers rgommers merged commit 77c78ae into scipy:main Apr 16, 2024
@h-vetinari h-vetinari deleted the compiler_bump branch April 16, 2024 06:28
@rgommers
Copy link
Member

I'm going to lower the minimum supported Clang version, this PR was too aggressive (see gh-20523 and #20524 (comment)).

@h-vetinari
Copy link
Member Author

I'm going to lower the minimum supported Clang version, this PR was too aggressive (see gh-20523 and #20524 (comment)).

Interesting. Seems I overlooked the remaining uses of macOS-11 images. The intention was to reflect the lowest version we're testing with, which I think is a reasonable policy (despite Homebrew).

@rgommers
Copy link
Member

The intention was to reflect the lowest version we're testing with, which I think is a reasonable policy (despite Homebrew).

Not quite. That's also not how we do it on Linux. Instead we look at what versions of GCC people still use, and then adjust the bound to that. That we don't have a separate "minimum Clang" job like we do a "minimum GCC" is a separate issue.

rgommers added a commit to rgommers/scipy that referenced this pull request Apr 19, 2024
The bump done in scipygh-20478 was too aggressive, and the info added to the
toolchain there was not fully correct. Also remove the sentence about
the lower bound not being enforced from the docs, since it is now.

Closes scipygh-20523

[skip cirrus]
@h-vetinari
Copy link
Member Author

That we don't have a separate "minimum Clang" job like we do a "minimum GCC" is a separate issue.

We're agreeing on all the principles, though to me this is not a separate issue. If we have ways to identify reasonable lower bounds for LLVM based on usage, then we need to add a CI job for that and keep it green. And then we're back on the same page with me saying "lower bound reflects the lowest version we're testing with".

@rgommers
Copy link
Member

Sure, agreed. That CI job would be nice to have.

rgommers added a commit that referenced this pull request Apr 19, 2024
…#20526)

The bump done in gh-20478 was too aggressive, and the info added to the
toolchain there was not fully correct. Also remove the sentence about
the lower bound not being enforced from the docs, since it is now.

Closes gh-20523

[skip cirrus]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure maintenance Items related to regular maintenance tasks Meson Items related to the introduction of Meson as the new build system for SciPy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants