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

Fix issue with partially failing unit-test test_sort_real_schur() #59

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

msmdev
Copy link
Owner

@msmdev msmdev commented Jul 21, 2023

The unit-test test_sort_schur() partially fails (works for M1 but not for M2 to M5) for some combinations of OS, python version, and slepc/noslepc. Strangely, the pattern of this failures is not constant over CI #378 to CI #382. Thus, my first assumption is that the issue might be related to changes in numpy introduced with version 1.25.0.

@msmdev msmdev self-assigned this Jul 21, 2023
@msmdev msmdev requested a review from michalk8 July 21, 2023 12:08
@msmdev
Copy link
Owner Author

msmdev commented Jul 21, 2023

Ok, so my assumption was indeed correct: the problem occured with changes from numpy 1.24.4 to 1.25.0.
Thus, enforcing numpy<1.25.0 solved the issue.
BUT: Are we happy with this restriction or do we need to put some more research into this?

@@ -1,3 +1,3 @@
docrep>=0.3.1
numpy>=1.19.0
numpy>=1.19.0,<1.25.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is too restrictive; based on your comment, this is hardware related (works on my M1), would rather skip the test for certain architectures and investigate further.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It is OS related I would say (not hardware related): with MacOS the test never fails but with Linux...
Or did you try it with Linux?

Copy link
Owner Author

Choose a reason for hiding this comment

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

(M1 to M5 are the test matrices used... nothing to do with hardware...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, sorry, just went too quickly through this PR. Seems really like a problem with the combination numpy>=1.25.0 and ubuntu, numpy==1.24.4 on ubuntu seems to work well (based on the 3.8 job).
Would consider skipping the combination using this combination.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I looked at the 1.25.0 release notes, it wasn't immediately obvious which change would be affecting this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

The only thing that looked suspicious to me when going through the numpy changelog was the change in np.r_[] and np.c_[]. We are using these in the troubled test.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It is indeed strange that numpy 1.25.0 worked with python 3.9 on ubuntu one time but on the other hand pinning numpy below 1.25.0 solved the problem in all cases. So there should be a connection...

Copy link
Owner Author

@msmdev msmdev Jul 30, 2023

Choose a reason for hiding this comment

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

Taking a closer look, I recognized that the case were 1.25.0 worked was the test using slepc, while 1.25.0 never succeeded without using slepc. But I don't see how this should affect the outcome of this test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants