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

Unpin Numpy #35232

Merged
merged 5 commits into from
Mar 10, 2023
Merged

Unpin Numpy #35232

merged 5 commits into from
Mar 10, 2023

Conversation

sf1919
Copy link
Contributor

@sf1919 sf1919 commented Feb 17, 2023

Description of work.
A few months ago we pinned numpy until we could resolve any issues with newer versions. This is the PR that fixes any issues and removes the pin.

To test:

  1. Check tests pass
  2. Code Review

@ajjackson - please can you take a look at the fix for the Abins tests?

There is no associated issue.

This does not require release notes because the changes should not affect users


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Are the release notes saved in a separate file, using Issue or PR number for file name and in the correct location?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

A couple of the Abins tests are using arrays in ragged state which raises a ValueError
e.g. ValueError: setting an array element with a sequence. The requested array has an
inhomogeneous shape after 1 dimensions

Adding dtype=object provides a temporary workaround
@sf1919 sf1919 added the ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS label Feb 17, 2023
@sf1919 sf1919 marked this pull request as draft February 17, 2023 11:23
@ajjackson
Copy link
Contributor

ajjackson commented Feb 17, 2023

As discussed I think the nicest way to fix the Abins tests is to move the array creation down into the
with self.assertRaises(ValueError): blocks. Then we are still checking that ragged arrays will be stopped, and don't really care whether it is Numpy or Abins that stops them.

I would suggest that we make a note to remove this check once we know Mantid is not being run on any older Numpy, but there is already a roadmap to remove these classes so it should happen naturally.

@@ -22,7 +22,7 @@ boost:
- 1.77

numpy:
- 1.19
- 1.24.*
Copy link
Member

Choose a reason for hiding this comment

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

Could I make a suggestion that we match conda-forge here unless there is anything that we need from a much more up to date verison? numpy is core to so many projects and we use a large set of libraries from conda-forge so having our minimum requires be the same gives us maximum compatibility.

Conda Forge maintain a pinning repository where they define these versions.

For numpy they define a version per Python version which is accomplished by matching the list entries for Python and numpy, e.g 3.8 uses 1.20.

@sf1919 sf1919 marked this pull request as ready for review February 22, 2023 11:45
@sf1919
Copy link
Contributor Author

sf1919 commented Feb 22, 2023

I've now made the changes requested by @martyngigg and @ajjackson .

@sf1919 sf1919 requested a review from thomashampson March 6, 2023 09:07
@robertapplin robertapplin self-assigned this Mar 6, 2023
Copy link
Contributor

@robertapplin robertapplin left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@thomashampson thomashampson merged commit 69d81f0 into main Mar 10, 2023
@thomashampson thomashampson deleted the unpin_numpy_Feb23 branch March 10, 2023 09:50
@jhaigh0 jhaigh0 mentioned this pull request May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants