Skip to content

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

Merged
bsipocz merged 1 commit into
astropy:masterfrom
dhomeier:dhomeier-ascii-tokenizer-erange
Dec 9, 2019
Merged

Changed OverflowError condition of fast ascii converter to >= |HUGE_VAL|#9738
bsipocz merged 1 commit into
astropy:masterfrom
dhomeier:dhomeier-ascii-tokenizer-erange

Conversation

@dhomeier

@dhomeier dhomeier commented Dec 8, 2019 •
edited by pllim
Loading

Copy link
Copy Markdown
Contributor

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

@astropy-bot astropy-bot Bot added the io.ascii label Dec 8, 2019
@pllim
pllim requested review from hamogu, olebole and taldcroft December 9, 2019 14:21
olebole
olebole approved these changes Dec 9, 2019
View reviewed changes

@olebole olebole left a comment

Copy link
Copy Markdown
Member

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.

hamogu
hamogu approved these changes Dec 9, 2019
View reviewed changes

@hamogu hamogu left a comment

Copy link
Copy Markdown
Member

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

pllim commented Dec 9, 2019

Copy link
Copy Markdown
Member

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

@bsipocz -- Does this need a change log?

@bsipocz

bsipocz commented Dec 9, 2019

Copy link
Copy Markdown
Member

@bsipocz -- Does this need a change log?

I feel it's not necessary? 🤷‍♀

taldcroft
taldcroft approved these changes Dec 9, 2019
View reviewed changes

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@bsipocz

bsipocz commented Dec 9, 2019

Copy link
Copy Markdown
Member

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
@bsipocz
Merge pull request #9738 from dhomeier/dhomeier-ascii-tokenizer-erange
96607ec 
Changed OverflowError condition of fast ascii converter to >= |HUGE_VAL|
@olebole

olebole commented Dec 11, 2019

Copy link
Copy Markdown
Member

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

Reviewers

@taldcroft taldcroft taldcroft approved these changes
@olebole olebole olebole approved these changes
@hamogu hamogu hamogu approved these changes

Assignees

No one assigned

Labels

io.ascii no-changelog-entry-needed

Projects

None yet

Milestone

v4.0

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

@dhomeier @pllim @bsipocz @olebole @taldcroft @hamogu