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

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

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

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).

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

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.

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

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

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
58 checks passed
@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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants