BUG: Fix dataframe.update not updating dtype #55509 v2
State: closed

- [x] closes #55509. 
- [x] 3 Tests added and passed.
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added an entry in the latest `doc/source/whatsnew/v3.0.rst`. (?)

A new PR was opened to replace the previous one (#55634) because a more idiomatic solution was found. The previous PR will be converted to draft. 

@rhshadrach @mroeschke 


Labels:
name: Bug
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

Comments:
Thanks for the contribution @aureliobarbosa. Can you add a note to the release notes please? You can check other PRs if you are not familiar with it.
Sure. For which version? 3.0? @datapythonista 
> Sure. For which version? 3.0? @datapythonista

Yes please
Done @datapythonista
Benchmarks from https://github.com/pandas-dev/pandas/pull/55634#discussion_r1375264127

```
size = 100_000
df1 = pd.DataFrame({'a': np.random.randint(0, 100, size)}, index=range(1, size+1))
df2 = pd.DataFrame({'a': np.random.randint(0, 100, 3)}, index=[10, 12, 11])
%timeit df1.update(df2)
# 1.11 ms ± 10.1 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)  <-- PR
# 2.32 ms ± 36.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)    <-- main

size = 100_000
index = list(range(1, size+1))
index[1], index[0] = index[0], index[1]
df1 = pd.DataFrame({'a': np.random.randint(0, 100, size)}, index=index)
df2 = pd.DataFrame({'a': np.random.randint(0, 100, 3)}, index=[10, 12, 11])
%timeit df1.update(df2)
# 1.05 ms ± 3.11 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)  <-- PR
# 1.76 ms ± 3.73 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)  <-- main

size = 100_000
df1 = pd.DataFrame({'a': np.random.randint(0, 100, 3)}, index=[10, 11, 12])
df2 = pd.DataFrame({'a': np.random.randint(0, 100, size)})
%timeit df1.update(df2)
# 517 µs ± 1.54 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)  <-- PR
# 318 µs ± 4.85 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)  <-- main

size = 10
df1 = pd.DataFrame({'a': np.random.randint(0, 100, size)})
df2 = pd.DataFrame({'a': [-1, -2, -3]}, index=[4, 4, 6])
df1.update(df2)
# ValueError: Update not allowed with duplicate indexes on other.  <-- PR
# ValueError: cannot reindex on an axis with duplicate labels      <-- main
```
/preview
Website preview of this PR available at: https://pandas.pydata.org/preview/pandas-dev/pandas/57637/
@datapythonista I am looking into ASV right now. Which kind of benchmark would be more useful: time, memory or both? Can you point me to a properly designed pandas benchmark so that I can have a reference for an start on this?

Independent of this, manual tests are going to be done locally and posted here for reference, in the following.
I think all our benchmarks should be reasonable. Understanding and running asv may be a bit tricky, but writing the benchmark should be trivial, just create some sample data and call `DataFrame.update` with it. I think checking time is probably good enough. The main complexity in my opinion is to use enough data so the timing is not super noise, but not have too much so the benchmark is slow.

Also, we use benchmarks more for the algorithms that we implement ourselves (like in C) that to check the performance of everything.  I think in this case it can be useful, but use your own judgement to decide whether it's useful or not.
@datapythonista 

> Do you think it's worth adding a use case to test when the indices have no intersection?

I agree because this is a behavior that was introduced on this PR. Added the test for this case and changed the message raise slightly to emphasize 'no intersection'.

> not sure if we can find a better name for the variable rows. Maybe mask or index_intersection is better.

The first of this PR used `indexes_intersection`, but then I changed to `rows` to simplify the notation. Now that you raised this point, I noted that a simple search on the file `frame.py` results in more than one hundred matches. Considering this, I agree with avoided using `rows`.
I did benchmark again the first three manual code tests proposed previously by @rhshadrach. Results are summarized below in the same order.

| Data | main  | PR  | 
| --- | --- | --- |
| monotonic  |  1.8 ms  | 1.29  ms  |  
| non-monotonic |  1.83 ms | 1.21 ms  |  
| small-frame-large-arg | 384 µs |595 µs | 

The performance gain is not as good as originally expected, but they are still relevant (28%~34% on large frames with small arguments!). @datapythonista Note that when the frame is small and the argument is large there is a performance loss, but as pointed out previously by @rhshadrach  this looks like an edge use case.
Hope to be done here. 

@datapythonista Next thing I will do is to try to implement an ASV benchmark for this function.

@rhshadrach I think that the take home message on this PR was that the function was manipulating data it wasn't expecting to touch on, and this case, the change in the dtype was the symptom for this problem.  

If you both know or find any bug that could be related or similar to this one, I would be glad to contribute.

Regards
Thanks @aureliobarbosa!