Skip to content

Conversation

davidhassell
Copy link
Contributor

Fixes #318

Notes:

@davidhassell davidhassell added the enhancement New feature or request label Feb 25, 2025
@davidhassell davidhassell added this to the Next release milestone Feb 25, 2025
Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

I am reviewing this together with the corresponding cf-python PR NCAS-CMS/cf-python#846, so please wait until I've submitted approval for both to merge anything, but I am happy with the code changes and functionality in cfdm itself hence approving (though I've raised some minor suggestions as usual) and both test suites pass so just doing final checks on the cf-python side now.

davidhassell and others added 4 commits February 27, 2025 08:28
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

My approval hasn't changed though I've found a few more means to investigate our numpy 2 compatibility, to be safe, going from the official migration guide.

First, I used ruff as an extra validation (though its reliability is uncertain) that we are numpy 2 compatible on both the cfdm branch here and the cf PR one:

$ ruff check cf/ --select NPY201                                                                ─╯
All checks passed!

I've also added np._set_promotion_state("weak_and_warn") to the __init.py__ files to see what warnings might emerge regarding the new type promotion behaviour.

Concerning this cfdm branch, I see a lot of them when running the test suite, though ironically thyey seem to emerge from numpy itself via numpy.ma. I do see one from our code, though:

..
/home/slb93/git-repos/cfdm/cfdm/data/subarray/mixin/pointtopology.py:41: UserWarning: result dtype changed due to the removal of value-based promotion from NumPy. Changed from int64 to int32.
  largest_node_id += 1
...

so please can you check that the behaviour of that line is safe and as intended in numpy 2. It looks harmless to me, but clearly something triggered the numpy warning so maybe those lines can be reformulated.

davidhassell and others added 2 commits February 27, 2025 12:53
@davidhassell
Copy link
Contributor Author

largest_node_id += 1

Certainly benign, but I've replaced with largest_node_id = largest_node_id + 1 to avoid potential confusion in the future: bd6be97

@davidhassell davidhassell merged commit c0c061d into NCAS-CMS:main Feb 28, 2025
@davidhassell davidhassell deleted the numpy-v2-from-cfa3 branch February 28, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to numpy version 2
2 participants