Skip to content

API: Add np.long and np.ulong #24922

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 16, 2023
Merged

API: Add np.long and np.ulong #24922

merged 4 commits into from
Oct 16, 2023

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented Oct 13, 2023 •
edited
Loading

Connected to #24224.

Hi @seberg,

In this minimal PR I introduced np.long and np.ulong only, without interfering with np.int_ and np.uint (the latter pair is an alias from now here but they map to NPY_LONG/NPY_ULONG exactly as before).

@mtsokol mtsokol self-assigned this Oct 13, 2023
@seberg
Copy link
Member

seberg commented Oct 13, 2023

Thanks, didn't realize it would be quite this small. Should probably add it to the stubs also? (I know very minimal, and doesn't help with the diff on the other PR.)

Maybe we can get this in quickly, then see if there are more small adaptations where np.long is clearly what we want to split out from the other PR (although I don't think it is terribly big, unfortunately the main moving parts are hard to split out).

@mtsokol
Copy link
Member Author

mtsokol commented Oct 13, 2023

Sure! Right, mypy stubs are missing.

eric-wieser
eric-wieser reviewed Oct 13, 2023
View reviewed changes
doc/release/upcoming_changes/24922.new_feature.rst Outdated
Comment on lines 4 to 5
`numpy.long` and `numpy.ulong` have been reintroduced as a direct
mapping to C's ``long`` and ``unsigned long`` types.
Copy link
Member

Choose a reason for hiding this comment

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

"reintroduced" suggests they have the same meaning as before they were removed, but this is not the case.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can just remove it and say: ... added as NumPy integers mapping to C's long and unsigned long. Prior to NumPy 1.24 np.long was an alias to Python's int.

It was actually deprecated at the time already I think. (Ignoring that it was Python long on Python 2, and then actually had the long long meaning in dtype(...), I think it was just int after that.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I updated it accordingly.

eric-wieser
eric-wieser reviewed Oct 13, 2023
View reviewed changes
numpy/_typing/_char_codes.py Outdated
Comment on lines 34 to 35
_UIntCodes = Literal["uint", "L", "=L", "<L", ">L"]
_ULongCodes = Literal["ulong"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_UIntCodes = Literal["uint", "L", "=L", "<L", ">L"]
_ULongCodes = Literal["ulong"]
_ULongCodes = Literal["ulong", "uint", "L", "=L", "<L", ">L"]

Copy link
Member Author

@mtsokol mtsokol Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that "uint" should still be associated with np.uint. I also wanted to make this PR minimal, just reintroduce np.long. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

These should be considered as aliases for the same thing, so I don't see why we'd want to give them a different typecode.

Copy link
Member

Choose a reason for hiding this comment

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

There is a tricky thing with the follow up, but right now, ulong is a correct type code for uint, so renaming it doesn't really matter. Maybe it actually makes sense to make: _UIntCodes = _ULongCodes?

What I am unclear about is what needs to change if we change this to _UIntCodes = _UIntpCodes later. @BvB93 gave a diff in the PR doing that, but it is not quite clear to me that it is complete now.

Copy link
Member Author

@mtsokol mtsokol Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I updated it so now:

_ULongCodes = Literal["ulong", "uint", "L", "=L", "<L", ">L"]
_UIntCodes = _ULongCodes

(same for int/long)

@mtsokol mtsokol requested a review from seberg October 16, 2023 11:48
@seberg
Copy link
Member

seberg commented Oct 16, 2023

Ok, let me put this in. I don't think this is the final word on typing here. Declaring it as an alias only is fine, but only of limited help for now and the alias isn't even used, yet.

In practice almost all (maybe even all) places should just use Long now.
The same goes for almost all uses of np.int_, although I suspect there are may be a few exception and maybe places where the None overload should move or use the _IntCodes alias explicitly.

@mtsokol do you want to follow up with that move? I am pretty sure changing what _IntCodes is here now without renaming practically everything is wrong. If this ends up renaming everything, then someone can eventually delete it again.

@seberg seberg merged commit 018c24d into numpy:main Oct 16, 2023
@mtsokol mtsokol deleted the introduce-np-long branch October 16, 2023 14:00
@mtsokol
Copy link
Member Author

mtsokol commented Oct 16, 2023

@mtsokol do you want to follow up with that move? I am pretty sure changing what _IntCodes is here now without renaming practically everything is wrong. If this ends up renaming everything, then someone can eventually delete it again.

Sure! I will take care of it.

@BvB93
Copy link
Member

BvB93 commented Oct 16, 2023 •
edited
Loading

Right, as (u)int is now merely a (u)intp alias I don't think there's much of a reason to keep the _(U)IntCodes type around (especially since the relevant string literals have been repurposed for _(U)LongCodes anyway).

In practice almost all (maybe even all) places should just use Long now.

This might be good to check, yes. There are a number of functions that always return int_-based dtypes as of 1.26, e.g. busday_count, but it's not entirely clear to me which ones now map to long and which ones to intp/int_ (wouldn't surprise me if it's a mix of both in reality...).

@seberg
Copy link
Member

seberg commented Oct 16, 2023

I think it is safe to say that they should all map to int_ now, unless the input is typed as "long". The only exception will be mtrand.
If there is a weird corner case where this turns out to be wrong, I am willing to shrug it off as a bug.

This was referenced Oct 30, 2023
Closed
Merged
@mtsokol mtsokol mentioned this pull request Jun 4, 2024
Merged
charliermarsh pushed a commit to astral-sh/ruff that referenced this pull request Jun 4, 2024
@mtsokol
Update NPY001 rule for NumPy 2.0 (#11735)
1645be0 
Hi!

This PR addresses #11093.

It skips `np.bool` and `np.long` replacements as both of these names
were reintroduced in NumPy 2.0 with a different meaning
(numpy/numpy#24922,
numpy/numpy#25080).
With this change `NPY001` will no longer conflict with `NPY201`. For
projects using NumPy 1.x `np.bool` and `np.long` has been deprecated and
removed long time ago, and accessing them yields an informative error
message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-wieser eric-wieser eric-wieser left review comments

@seberg seberg Awaiting requested review from seberg

Assignees

@mtsokol mtsokol

Labels
30 - API Numpy 2.0 API Changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mtsokol @seberg @BvB93 @eric-wieser