Skip to content

Conversation

NevroHelios
Copy link

@NevroHelios
Copy link
Author

Hi @jbrockmendel ,
I’ve updated the code so that it now preserves the original dtype. Because of this, some of the existing type-checking tests are failing — they were written for the old behavior. These tests might need to be updated to reflect the new semantics. Could you let me know if adjusting them in this way makes sense?

@jbrockmendel
Copy link
Member

Please use your best judgement.

@NevroHelios
Copy link
Author

Okay, thanks.. I’ll update the tests to keep things consistent and aligned with user expectations.

def map(self, mapper, na_action: Literal["ignore"] | None = None):
def map(self, mapper,
na_action: Literal["ignore"] | None = None,
preserve_dtype: bool = False):
Copy link
Member

Choose a reason for hiding this comment

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

why is a new keyword getting added?

Copy link
Member

Choose a reason for hiding this comment

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

i see, you're trying to avoid changing any old behavior. in this case, the new behavior should be generally better than the old behavior. if you find cases where it isn't, those likely represent bugs in _cast_pointwise_result.

Copy link
Author

Choose a reason for hiding this comment

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

while trying to put the _casr_pointwise_result in map and updating the test cases, like you mentioned, I faced some bugs. It was not fully implemented for some particular niche types. (truediv , rtruediv or ns, ms ) and while I don't remember all the types.. this function definitely needs few more updates...

Copy link
Member

Choose a reason for hiding this comment

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

If you find other bugs, please open issues about them

self,
mapper,
na_action: Literal["ignore"] | None = None,
preserve_dtype: bool = False,
Copy link
Member

Choose a reason for hiding this comment

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

we don't want this new keyword.

Copy link
Author

Choose a reason for hiding this comment

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

as the current implementation is not working for all the types, do you have a suggestion on how I should approach this without introducing the new keyword?

Copy link
Member

Choose a reason for hiding this comment

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

same as i said here #62177 (comment)

self,
n: PositionalIndexer | tuple,
dropna: Literal["any", "all", None] = None,
dropna: Literal["any", "all"] | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

pls avoid unrelated changes

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, but due to a ci check on formatting, I used ruff and it changed few files. Thanks for pointing out, I will revert it back.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REF: use _cast_pointwise_result in map

2 participants