Skip to content

BUG: fixed to_numeric loss in precision when converting decimal type to integer #57213 #58139

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

Closed
wants to merge 1 commit into from
Closed

BUG: fixed to_numeric loss in precision when converting decimal type to integer #57213 #58139

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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v3.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ Performance improvements
Bug fixes
~~~~~~~~~
- Fixed bug in :class:`SparseDtype` for equal comparison with na fill value. (:issue:`54770`)
- Fixed bug in :func:`to_numeric` that resulted in precision loss when converting decimal type to integer. (:issue:`57213`)
- Fixed bug in :meth:`.DataFrameGroupBy.median` where nat values gave an incorrect result. (:issue:`57926`)
- Fixed bug in :meth:`DataFrame.cumsum` which was raising ``IndexError`` if dtype is ``timedelta64[ns]`` (:issue:`57956`)
- Fixed bug in :meth:`DataFrame.join` inconsistently setting result index name (:issue:`55815`)
Expand Down
13 changes: 12 additions & 1 deletion pandas/core/tools/numeric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

from decimal import Decimal
from typing import (
TYPE_CHECKING,
Literal,
Expand Down Expand Up @@ -209,6 +210,7 @@ def to_numeric(
values = values.view(np.int64)
else:
values = ensure_object(values)
old_values = values
coerce_numeric = errors != "raise"
values, new_mask = lib.maybe_convert_numeric( # type: ignore[call-overload]
values,
Expand Down Expand Up @@ -255,7 +257,16 @@ def to_numeric(
for typecode in typecodes:
dtype = np.dtype(typecode)
if dtype.itemsize <= values.dtype.itemsize:
values = maybe_downcast_numeric(values, dtype)
try:
if (len(old_values) != 0) and isinstance(
old_values[0], Decimal
Copy link
Member

@mroeschke mroeschke Apr 4, 2024

Choose a reason for hiding this comment

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

I think this fix is too specific.

Copy link
Author

@Leventide Leventide Apr 4, 2024

Choose a reason for hiding this comment

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

Excuse me, but i'm confused as to what you mean by too specific, the bug was specifically about the decimal type no?
I'm sorry but I'm pretty new to open source contributing.

Copy link
Contributor

@aureliobarbosa aureliobarbosa Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Leventide!

I previously looked into this issue and thought about working on it! So, I think it is great that you gave it a shot and started working on a fix for it. I am also a new contributor to open source and have to confess that it was a little bit tricky to get my first PR accepted. Since this seems to be your first contribution, let's talk about a few things that can possibly help you go forward (those are reflections that helped me keep my effort to contribute and learn in this community).

  1. Context: If you look into the size of this project (in terms of issues, for example) and track the number of active maintainers actually working on it you will realize that they are completely overwhelmed by work: there is a huge list of bugs, issues doesn't stop to arrive and they need to evolve the code toward current goals. Just take a look on PDEP and on the list of issues for 3.0 release. Besides, there is a lot of people trying to enter in the community and, among the skilled one, few are willing or have the proper skills to mentor new members. In this direction, I would recommend you to watch this talk Brett Cannon: Setting Expectations for Open Source Participation. Considering this, do not take it to the personal side if a core member take a few months to review your PR, this is actually very common. It takes some time for us to understand the details of the library, the social codes of the community, and it also takes some time for the members of the community to know new members.

  2. What happens if your code is accepted: pandas is huge (by any measure). Any changes you make into the code will eventually be used by a lot of people and, your contribution being accepted, someone will have to maintain and evolve it (including fixing new bugs, regressions, etc.). You got see yourself in the place of a maintainer to understand the responsibility of accepting a PR (even one that is passing all tests!).

  3. You got a trick problem for a first issue! This is not the first issue on pandas and numpy with problems when trying round-trip a big integer number. There could be many strategies for attacking this problem, but I would check into the representation of floating points on python, would read a little bit more about representing integers on this ecosystem, and would follow the tip given by @rhshadrach on the original issue.

  4. Your code: I looked briefly into the function and also into your code. Probably @mroeschke wanted tell you is that it is necessary to make the logic as general as possible, without introducing too much specificity in the code. Well, you may think that this piece of code already is too much specific, that it is the way it is... nevertheless, things ended up how it is for historical reasons and the idea is, as far as possible, improve the code, simplifying its logic. This is my point of view. Regarding you code, it introduces three new levels of nesting: a try-except (which doesn't seem to used in the current context within this library), and two nested if-else blocks. I think this is something that could be avoided. Maybe there is some intrinsic pattern for capturing the data type and, for some reason, the data is falling in the wrong path... maybe the problem is in the function maybe_downcast_numeric, or it is being used out of context. Those are possibilities. Possibilities are so much that I would recommend debugging it and following through step by step. In any way, maybe you already did it.

Well, it turned out that I was planning to write about the first three points and they ended up appearing here. Hope that you keep on trying this PR, continue contributing, and learning.

Regards

):
if dtype.itemsize == values.dtype.itemsize:
values = np.array(old_values, dtype=dtype)
else:
values = maybe_downcast_numeric(values, dtype)
except NameError:
values = maybe_downcast_numeric(values, dtype)

# successful conversion
if values.dtype == dtype:
Expand Down
7 changes: 7 additions & 0 deletions pandas/tests/tools/test_to_numeric.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,3 +904,10 @@ def test_coerce_pyarrow_backend():
result = to_numeric(ser, errors="coerce", dtype_backend="pyarrow")
expected = Series([1, 2, None], dtype=ArrowDtype(pa.int64()))
tm.assert_series_equal(result, expected)


def test_decimal_precision():
df = DataFrame({"column1": [decimal.Decimal("1" * 19)]})
result = to_numeric(df["column1"], downcast="integer")
expected = df["column1"].astype("int64")
tm.assert_series_equal(result, expected)