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

Relax numpy upper version cap #1172

Merged
merged 7 commits into from
Apr 27, 2024
Merged

Conversation

mtreinish
Copy link
Member

In #1012 we added an upper version cap to numpy to prevent it from installing numpy 2.0 before we confirmed that rustworkx was compatible with it. Now that numpy 2.0.0rc1 has been released we're able to confirm that rustworkx works fine with numpy 2.0. This commit raises the upper bound on the numpy version to < 3 to enable installing numpy 2.0 with rustworkx.

In Qiskit#1012 we added an upper version cap to numpy to prevent it from
installing numpy 2.0 before we confirmed that rustworkx was compatible
with it. Now that numpy 2.0.0rc1 has been released we're able to confirm
that rustworkx works fine with numpy 2.0. This commit raises the upper
bound on the numpy version to < 3 to enable installing numpy 2.0 with
rustworkx.
Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for confirming

While we didn't have any test coverage for this looking at the numpy 2.0
migration guide one thing we'll have to handle is the new copy kwarg on
array:

https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword

This commit updates the sole use of __array__ we have on custom sequence
return types so that if copy=False is passed in we raise a ValueError.
Additionally, the dtype handling is done directly in the rustworkx code
now to ensure we don't have any issues with numpy 2.0.
@mtreinish
Copy link
Member Author

In talking with @jakelishman (who handled the numpy 2.0 compatibility for Qiskit) he pointed out that the __array__ method has new arguments which we weren't handling in rustworkx. The tests wouldn't cover this because even when running with numpy 2.0.0rc1 we weren't touching that. I updated the method in: fc8cb4b to handle the semantics from the migration guide.

@coveralls
Copy link

coveralls commented Apr 24, 2024

Pull Request Test Coverage Report for Build 8855732656

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.008%) to 96.521%

Files with Coverage Reduction New Missed Lines %
rustworkx-core/src/generators/random_graph.rs 2 79.43%
Totals Coverage Status
Change from base Build 8826490220: -0.008%
Covered Lines: 17338
Relevant Lines: 17963

💛 - Coveralls

@mtreinish mtreinish added this to the 0.15.0 milestone Apr 25, 2024
Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

Can we refactor the dtype handling to use py.import_bound() and then handle the NumPy module as a traditional Python object to call the methods?

I'd rather avoid eval_bound and __import__ in the same code

src/iterators.rs Outdated Show resolved Hide resolved
src/iterators.rs Outdated Show resolved Hide resolved
src/iterators.rs Outdated Show resolved Hide resolved
@IvanIsCoding IvanIsCoding merged commit a6c9849 into Qiskit:main Apr 27, 2024
28 checks passed
@mtreinish mtreinish deleted the relax-numpy-cap branch April 27, 2024 01:27
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.

None yet

3 participants