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

DEPS: Update reprs with numpy NEP51 scalar repr #54268

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

mroeschke
Copy link
Member

@mroeschke mroeschke added CI Continuous Integration Dependencies Required and optional dependencies labels Jul 26, 2023
@mroeschke mroeschke requested a review from WillAyd as a code owner July 26, 2023 17:02
@mroeschke mroeschke added this to the 2.1 milestone Jul 26, 2023
@@ -5250,7 +5250,8 @@ def _raise_scalar_data_error(cls, data):
# in order to keep mypy happy
raise TypeError(
f"{cls.__name__}(...) must be called with a collection of some "
f"kind, {repr(data)} was passed"
f"kind, {repr(data) if not isinstance(data, np.generic) else str(data)} "
Copy link
Member

Choose a reason for hiding this comment

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

does str work unconditionally?

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 according to the linked PR seems so

Copy link
Member

Choose a reason for hiding this comment

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

the can we simplify this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry, str works unconditionally for numpy scalars but repr vs str would make this display differently for other scalars e.g. python strings

@@ -160,6 +161,9 @@ def _empty(cls, shape: Shape, dtype: ExtensionDtype):
)
return result

def _formatter(self, boxed: bool = False) -> Callable[[Any], str | None]:
return super()._formatter(boxed=True)
Copy link
Member

Choose a reason for hiding this comment

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

we're ignoring the boxed keyword?

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 since I think we'll need to always return str given the NEP 51 change. Actually probably just simpler to return str here

name = type(self).__name__
repr_str = f"{name}({repr(left)}, {repr(right)}, closed={repr(self.closed)})"
repr_str = f"{name}({disp(left)}, {disp(right)}, closed={repr(self.closed)})"
Copy link
Member

Choose a reason for hiding this comment

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

maybe the thing to do is unpack numpy scalars at an earlier stage?

Copy link
Member Author

Choose a reason for hiding this comment

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

But we should still store numpy scalars for left/right on Interval right?

Copy link
Member

Choose a reason for hiding this comment

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

i was thinking store native scalars

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm might make sense, not sure if there's any larger ramifications of that chance. Might be better to experiment in a follow up?

Copy link
Member

Choose a reason for hiding this comment

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

sure

@jbrockmendel
Copy link
Member

This will get the CI back to green?

@mroeschke
Copy link
Member Author

This will get the CI back to green?

Yup. I guess we can merge to get back to green and follow up if needed.

@mroeschke mroeschke merged commit e51216c into pandas-dev:main Jul 28, 2023
@mroeschke mroeschke deleted the ci/failure branch July 28, 2023 17:11
@mroeschke mroeschke mentioned this pull request Mar 7, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Dependencies Required and optional dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants