Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

@garrettwrong garrettwrong commented Oct 10, 2024

Numpy 2 requires more changes than I recall. I'm thinking maybe I didn't actually get this far before due to depends issues. My mistake.

Because a lot of scientific python libraries are not supporting Numpy2 yet (I can see why, don't hold your breathe!), we should plan to support both 1 and 2 for a while. (See numpy/numpy#26191) . This PR adds that compatibility for copy= behavior which was one of ASPIRE's main blockers. ( grumbles, I understand the ABI compat issues, but I truly don't understand how user API bridges could not have been built ....) Update: I was able to resolve all occurrences covered by tests without the workaround.

There are many dtype issues. I've resolved those covered by unit tests. One I've noted I wanted to come back to, as I'm not satisfied yet (I figured out where to fix it, but not where it came from 😬 ... 🔎 ). I'll look at that before review, but wanted to get this draft up now to run the CI etc.

My main concern is that I can virtually guarantee I'm missing some np2 dtype mismatches that will cause runtime crashes later. Some of these are not going to be from our code (they will likely be fluid as extern packages update). There isn't much I can do about that other than to try and fix them on our side as they come to surface. 🤷‍♂️

@garrettwrong garrettwrong added enhancement New feature or request CI Continuous Integration extern Relating to external changes dependencies Pull requests that update a dependency file labels Oct 10, 2024
@garrettwrong garrettwrong self-assigned this Oct 10, 2024
@codecov
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.39%. Comparing base (ab3dec1) to head (5e75e20).
Report is 14 commits behind head on develop.

Files with missing lines Patch % Lines
src/aspire/reconstruction/kernel.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1194   +/-   ##
========================================
  Coverage    90.38%   90.39%           
========================================
  Files          132      132           
  Lines        13754    13761    +7     
========================================
+ Hits         12432    12439    +7     
  Misses        1322     1322           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@garrettwrong garrettwrong force-pushed the np2 branch 2 times, most recently from b86318b to 691b464 Compare October 22, 2024 12:35
@garrettwrong
Copy link
Collaborator Author

I'm going to enable this for review just to run the conda CI jobs. Still a draft... at this time not intending to merge.

@garrettwrong garrettwrong marked this pull request as ready for review October 25, 2024 12:51
@garrettwrong garrettwrong requested a review from janden as a code owner October 25, 2024 12:51
@garrettwrong garrettwrong marked this pull request as draft October 25, 2024 13:43
@garrettwrong
Copy link
Collaborator Author

@j-c-c , lets get this reviewed and into develop asap so we can start running with it to shake out more issues, thanks.

Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

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

LGTM

@garrettwrong garrettwrong marked this pull request as ready for review November 18, 2024 18:20
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

LGTM

@garrettwrong garrettwrong merged commit f36f51b into develop Nov 20, 2024
44 checks passed
@garrettwrong garrettwrong deleted the np2 branch November 20, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous Integration dependencies Pull requests that update a dependency file enhancement New feature or request extern Relating to external changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants