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

MNT: Enable numpy 2+ installs #758

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

greglucas
Copy link
Collaborator

Change Summary

Overview

Numpy 2 is more strict about the overflow math calculations, so we needed to update a few cases to handle that as well.

New Dependencies

Bumping numpy to allow 2+

Numpy 2 is more strict about the overflow math calculations, so
we needed to update a few cases to handle that as well.
@@ -336,6 +336,10 @@ def to_signed16(n: int) -> int:
primary_vectors = []
secondary_vectors = []

# To avoid overflows, we need to cast the potentially 8 bit signed integers to
# int32 before the bitshifting operations below.
vector_data = vector_data.astype(np.int32)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maxinelasp, I'm not sure how you want to handle this, so let me know if you want something else. You are calling astype(int32) in the code calling section, but in the tests you pass in a frombuffer() call which is only int8. We could get rid of the type-cast in the calling routine and rely on this, or we could immediately cast the frombuffer in the test too if you'd prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Annoyingly, this is also complicated by the compression code that I'm working on, which also takes in vector data. Is the problem that the 8 bit ints are signed?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this change passes the tests then I'm ok with it. The bitshifting is unfortunately complicated with different types, but I am also looking to rework the types to be more consistent in my newest change, so that it works with the uncompressed and compressed algorithms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the problem that the 8 bit ints are signed?

The problem is here:

x = (
((vector_data[pos + 0] & 0xFF) << 8)
| ((vector_data[pos + 1] & 0xFF) << 0)
) & 0xFFFF

where you are doing math with 0xFFFF which is out of range for those types, so numpy2+ is warning and saying we need to be explicit with what types we want to do the math on.

Within the code you are already casting here:

mag_l0.VECTORS.astype(dtype=np.int32), # type: ignore[union-attr]

But in the tests, you aren't:

(primary_vectors, secondary_vectors) = MagL1a.process_vector_data(
input_data, total_primary_vectors, total_secondary_vectors
)

So I was saying that we could also update this in those two locations if you wanted as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, ok, in that case either change is fine, and I'll go back once the compression algorithm is done and make sure it's consistent with the types I'm using there.

@greglucas greglucas added the enhancement New feature or request label Aug 20, 2024
Copy link
Contributor

@subagonsouth subagonsouth 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.

@@ -38,7 +38,7 @@ space_packet_parser = ">=4.2.0"
spiceypy = ">=6.0.0"
xarray = '>=2023.0.0'
pyyaml = "^6.0.1"
numpy = "^1.26.4"
numpy = "<=3"
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to have this pinned to a minor version, no? numpy = "^2.1.0 I'm not sure we want random bumps up to, for example, 2.2.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we should require numpy 2+ because that will likely cause issues with other packages only supporting <=2 because those packages maybe haven't done the fixes themselves yet to be compatible.

I'm personally a fan of more open-ended ranges for packages so that we don't get pinned back and then have to do a massive update later on. When deploying/installing the package as a user, then we can/should use explicit package locks, but in general I have a preference for leaving the testing of the package open so that we don't get stuck way behind the times and miss things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, since this uses a lockfile I'm alright with that. Did you see if requiring numpy 2+ actually caused issues with packages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it does not cause issues right now, we are able to install numpy 2+. I think this is a difference of whether we are the final "user" (what I think you're getting at with the lockfile for install) or if we expect this package to be used by others (my assumption) and potentially installing other packages besides the ones we require. Pinning to numpy 2+ will affect others and not just ourselves. If we work with numpy 1.26 right now we should let that keep working IMO.

@greglucas
Copy link
Collaborator Author

Merging now, we can follow up on installation pins in subsequent PRs.

@greglucas greglucas merged commit 0135dcc into IMAP-Science-Operations-Center:dev Aug 20, 2024
17 checks passed
@greglucas greglucas deleted the np2-upgrade branch August 20, 2024 17:57
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants