8000 MAINT: dtype.name works for NEP 42 dtypes by ngoldbaum · Pull Request #23047 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: dtype.name works for NEP 42 dtypes #23047

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

Merged
merged 1 commit into from
Jan 20, 2023
Merged

MAINT: dtype.name works for NEP 42 dtypes #23047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion numpy/core/_dtype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,8 @@ def _name_includes_bit_suffix(dtype):
elif dtype.type == np.bool_:
# implied
return False
elif dtype.type is None:
return True
elif np.issubdtype(dtype, np.flexible) and _isunsized(dtype):
# unspecified
return False
Expand All @@ -348,7 +350,9 @@ def _name_get(dtype):
# user dtypes don't promise to do anything special
return dtype.type.__name__

if issubclass(dtype.type, np.void):
if dtype.kind == '\x00':
Copy link
Member

Choose a reason for hiding this comment

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

We really should improve thi. Right now I have type(dtype)._parametric and _abstract. I am thinking it might make sense to make these public and (maybe without the underscore).
Could also add it to the dtype instances. I think we can consider these attributes stable API really (maybe even already with that underscore).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think having a concrete and obvious way to signal that this is a new-style dtype would be great. I don't have a strong opinion about how that should be spelled.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion about how that should be spelled.

If not you, who then? :). More seriously though, we have to make these public in some form I think. And if we are unsure, a safe bet is to just keep it as is (with an underscore), but define it as stable API (and maybe document it).

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a logical test on dtype._parametric and dtype._abstract that can be used to get the same information as checking dtype.kind == '\x00'?

Because there are legacy parametric and abstract dtypes I'm not sure there is, so making these attributes public is might be orthogonal to this discussion.

That said, I agree making them public would be useful.

Maybe just adding a new check in arraydescr_isbuiltin_get and allow dtype.isbuiltin to be 3? If new public attributes on dtype instances are OK, it could also be a new attribute which is only True for dtypes are created through the public dtype API. I guess naming is a tad awkward because of the existence of legacy user dtypes, but something like dtype.is_nonlegacy_custom maybe?

Copy link
Member

Choose a reason for hiding this comment

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

Returning 3 there seems OK, the only danger is if someone out there did dtype.isbuiltin != 2 or something crazy. But that seems unlikely?! dtype.iscustom is maybe not so bad (I would just ignore the "legacy" part and assume legacy dtypes will die off eventually?)

I do think we need this information also on the type probably in the long term. One other way to do this is always isinstance/issubclass(..., np.UserDType). That would probably make Marten very happy I am sure ;). I do not see that such APIs as mutual exclusive, though.

name = type(dtype).__name__
elif issubclass(dtype.type, np.void):
# historically, void subclasses preserve their name, eg `record64`
name = dtype.type.__name__
else:
Expand Down
3 changes: 3 additions & 0 deletions numpy/core/tests/test_custom_dtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ def test_repr(self):
# Check the repr, mainly to cover the code paths:
assert repr(SF(scaling=1.)) == "_ScaledFloatTestDType(scaling=1.0)"

def test_dtype_name(self):
assert SF(1.).name == "_ScaledFloatTestDType64"

@pytest.mark.parametrize("scaling", [1., -1., 2.])
def test_sfloat_from_float(self, scaling):
a = np.array([1., 2., 3.]).astype(dtype=SF(scaling))
Expand Down
0