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

BUG, ENH: fix array2string rounding bug by adding min_digits option #18629

Merged
merged 5 commits into from Mar 31, 2021

Conversation

ahaldane
Copy link
Member

@ahaldane ahaldane commented Mar 16, 2021

Fixes #18609

This PR adds a new min_digits argument to the dragon4 float printing methods, such as np.format_float_positional. This kwd guarantees that at least the given number of digits will be printed, when printing in unique=True mode, even if the extra digits are unnecessary to uniquely specify the value. It is the counterpart to the precision argument which sets the maximum number of digits to be printed. (When unique=False fixed-precision mode, it has no effect, and the precision arg fixes the number of digits)

Using this new keyword, I then fix a bug in arrayprint ( #18609). The bug in the old code is that, in order to guarantee that all elements of an array print using the same number of digits, we turned off unique=True because we needed to use fixed-precision mode to fix the number of digits. However, unique mode rounds differently from fixed-precision mode, so the rounding was wrong. This PR instead now stays in unique mode to print the elements, but uses the min_digits and precision arguments to fix the number of output digits.

It looks like a lof of chages, but a lot of it is just introducing the min_digits arg into all the function signatures in dragon4 and in the user api.

Copy link
Member

@BvB93 BvB93 left a comment

Choose a reason for hiding this comment

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

Can you update the annotations in the arrayprint.pyi stub file?
By the looks of it the type of min_digits should be Optional[int].

def format_float_scientific(
x: _FloatLike_co,
precision: Optional[int] = ...,
unique: bool = ...,
trim: Literal["k", ".", "0", "-"] = ...,
sign: bool = ...,
pad_left: Optional[int] = ...,
exp_digits: Optional[int] = ...,
) -> str: ...
def format_float_positional(
x: _FloatLike_co,
precision: Optional[int] = ...,
unique: bool = ...,
fractional: bool = ...,
trim: Literal["k", ".", "0", "-"] = ...,
sign: bool = ...,
pad_left: Optional[int] = ...,
pad_right: Optional[int] = ...,
) -> str: ...

@ahaldane ahaldane force-pushed the min_digits branch 2 times, most recently from b510396 to ed3a024 Compare March 17, 2021 14:10
@seberg seberg self-requested a review March 18, 2021 02:12
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks Allan. I took the liberty to rebase, if we want a backport give me a ping and I will do that.

I won't claim the have followed all the logic in dragon4.c but the code changes look all good and the tests sufficiently thorough.

numpy/core/arrayprint.py Outdated Show resolved Hide resolved
numpy/core/arrayprint.py Outdated Show resolved Hide resolved
ahaldane and others added 3 commits March 19, 2021 12:42
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@charris charris changed the title BUG/ENH: fix array2string rounding bug by adding min_digits option BUG, ENH: fix array2string rounding bug by adding min_digits option Mar 21, 2021
@charris
Copy link
Member

charris commented Mar 21, 2021

Could use a release note.

@charris charris added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 23, 2021
@charris charris merged commit 2afcdbf into numpy:main Mar 31, 2021
@charris
Copy link
Member

charris commented Mar 31, 2021

Thanks Allan

@charris charris removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 31, 2021
charris added a commit to charris/numpy that referenced this pull request Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

floatmode=unique sometimes giving wrong rounding
4 participants