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

Changed OverflowError condition of fast ascii converter to >= |HUGE_VAL| #9738

Merged
merged 1 commit into from
Dec 9, 2019

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented Dec 8, 2019

Addressing #9694 - if we are right about the fast C converter internally calculating in 80 bit on some architectures like x86_64/gcc 9.2 (see discussion there), this will still raise an OverflowError when the returned value exceeds np.float64 range (and becomes inf).

Fix #9694

Copy link
Member

@olebole olebole left a comment

Choose a reason for hiding this comment

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

This is exactly the change @dhomeier suggested before. I successfully tested this on our i386 architecture. What I did not test yet is whether it does harm on other platforms; however it looks simple enough for me.
I am planning to upload the RC2 plus this patch to Debian experimental later today (CET), so if a definitive confirmation for all our platforms is needed, I can give that only tomorrow.

Copy link
Member

@hamogu hamogu left a comment

Choose a reason for hiding this comment

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

This is way beyond my C knowledge, but I don't see how this change could do any harm, so I'm fine with including it since it seems to work.

@pllim pllim added this to the v4.0.1 milestone Dec 9, 2019
@pllim
Copy link
Member

pllim commented Dec 9, 2019

Okay, let's wait for a follow-up from @olebole before merge.

@bsipocz -- Does this need a change log?

@bsipocz
Copy link
Member

bsipocz commented Dec 9, 2019

@bsipocz -- Does this need a change log?

I feel it's not necessary? 🤷‍♀

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

👍

@bsipocz
Copy link
Member

bsipocz commented Dec 9, 2019

Thanks @dhomeier!

@bsipocz bsipocz merged commit 98d707b into astropy:master Dec 9, 2019
@bsipocz bsipocz modified the milestones: v4.0.1, v4.0 Dec 9, 2019
bsipocz added a commit that referenced this pull request Dec 11, 2019
Changed OverflowError condition of fast ascii converter to >= |HUGE_VAL|
@olebole
Copy link
Member

olebole commented Dec 11, 2019

It is already closed, but I just wanted to confirm that this does not harm on our other platforms ;-)

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.

4.0 rc1: Not enough warnings on i386 in test_data_out_of_range
6 participants