-
-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
BUG: update should try harder to preserve dtypes #4094 #40219
Conversation
Check out this comment for more information pandas-dev#40219 (comment)
@@ -6963,7 +6964,11 @@ def update( | |||
if mask.all(): | |||
continue | |||
|
|||
self[col] = expressions.where(mask, this, that) | |||
col_array = expressions.where(mask, this, that) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are you trying to do here? infer and cast? this is very awkward what you are doing.
.where casts to the most appropriate type, but then you are casting back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.reindex_like
changes the dtype to a nullable dtype if other.index
is a subset of self.index
.
.where
receives a nullable dtype and continues to use it, even though the NaNs are masked.
I catch the case where self[col] and other[col] originally have the same dtype, but .where
returns a different dtype.
With the assumption, that all NaNs are masked I am casting back, yes.
Example:
self[col] = Series(["a", "b"], dtype=str)
other[col] = Series(["c"], dtype=str)
.reindex -> other[col] = Series(["c", None], dtype=object)
.where -> self[col] = Series(["c", "b"], dtype=object)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is .reindex_like
called? this should be fixed at the sort not patched over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reindex_like is called here: https://github.com/pandas-dev/pandas/pull/40219/files#diff-421998af5fe0fbc54b35773ce9b6289cae3e8ae607f81783af04ebf1fbcaf077R6943 to have the same index in the other as in self, but for missing indices it inserts NANs and changes data type, maybe reindex_like needs to be changed in this case...
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
@Horstage closing as stale. ping when ready to continue |
I have to admit, I am not quite sure about the code in the initial example. But the rest of the given examples (1, 2) are converted to tests and they do pass.
I would have loved to make better use of hypothesis in the tests, but currently column does not support dtype strategies and neither does from_dtype (if I would define the elements of the column and infere the dtype).