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

kernel: ensure uniform printing of macfloats nan, inf, -inf #2274

Merged
merged 3 commits into from
Mar 24, 2018

Conversation

fingolfin
Copy link
Member

On some platforms, NaN values might be printed as nan, on others as NaN;
on some, the sign of the Nan value may be printed, on other not so (and the
sign of a NaN is something which highly depends on the float implementation to
start with).

This made it difficult to write tests which rely on the way these values are
printed. Hence we now deal with these values explicitly and in a manner which
produces identical results across all platforms.

Fixes #2193

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them topic: kernel labels Mar 21, 2018
@codecov
Copy link

codecov bot commented Mar 21, 2018

Codecov Report

Merging #2274 into master will decrease coverage by <.01%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #2274      +/-   ##
==========================================
- Coverage   71.44%   71.44%   -0.01%     
==========================================
  Files         479      479              
  Lines      251654   251661       +7     
==========================================
+ Hits       179798   179801       +3     
- Misses      71856    71860       +4
Impacted Files Coverage Δ
src/macfloat.h 100% <ø> (ø) ⬆️
src/macfloat.c 79.26% <76.92%> (+4.74%) ⬆️
hpcgap/lib/hpc/queue.g 66.4% <0%> (-3.2%) ⬇️
src/gap.c 63.24% <0%> (-0.34%) ⬇️
src/hpc/threadapi.c 37.22% <0%> (-0.2%) ⬇️

@dimpase
Copy link
Member

dimpase commented Mar 22, 2018

OK, this does give inf on GAP built with vanilla gcc 7.2 on SPARC Solaris 11, good.

Is there a way to test NaN's from GAP?

@fingolfin
Copy link
Member Author

You mean a way to generate a NaN, like 0.0/0.0 ? Or the IsNaN property to test for a nan?

@dimpase
Copy link
Member

dimpase commented Mar 22, 2018

Right, this works (with the branch here) on SPARC Solaris

gap> Float(0.0/0.0);
nan

Would you mind adding tests for this in tst/testinstall/float.tst ?
There are only tests for inf there.

Copy link
Contributor

@stevelinton stevelinton left a comment

Choose a reason for hiding this comment

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

Looks good to me. Should we document this, since it might just surprise someone who system usually prints NoN?

@fingolfin
Copy link
Member Author

@stevelinton so far, we only know of one system which does not print nan, namely SPARC Solari 11. I highly doubt that many people use GAP there (or that many people in mathematics still use such systems). And of those, even fewer will use floats inside GAP. And of those, even fewer will ever see a nan... My guess is that the number is 1, and consists precisely of @dimpase (thanks for catching this, BTW!!!) but of course I could be wrong ;-).

So, I'd rather not spend time on documenting this. Of course on the long run, it would be nice to document how nan and inf etc. are printed in general, but that's part of the bugger "floating points in GAP need to be better documented" issue and so IMHO beyond the scope of this PR...

@dimpase
Copy link
Member

dimpase commented Mar 23, 2018

My guess is that the number is 1, and consists precisely of @dimpase (thanks for catching this, BTW!!!) but of course I could be wrong ;-).

It's bigger than 1, e.g. @jdemeyer can also be considered counted in :-) but not that we know more.

I'd reiterate that printing nan in at least one test sounds like a reasonable thing, though.

@fingolfin fingolfin force-pushed the mh/float-inf branch 2 times, most recently from e3bd632 to 780f81f Compare March 23, 2018 19:31
@fingolfin
Copy link
Member Author

I added some tests and also made SIGN_MACFLOAT and SIGNBIT_MACFLOAT consistent

On some platforms, NaN values might be printed as `nan`, on others as `NaN`;
on some, the sign of the Nan value may be printed, on other not so (and the
sign of a NaN is something which highly depends on the float implementation to
start with).

This made it difficult to write tests which rely on the way these values are
printed. Hence we now deal with these values explicitly and in a manner which
produces identical results across all platforms.

Fixes gap-system#2193
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks!

@fingolfin fingolfin merged commit 3f89d62 into gap-system:master Mar 24, 2018
@jdemeyer
Copy link

I'd reiterate that printing nan in at least one test sounds like a reasonable thing, though.

I agree. At least it checks that GAP doesn't crash when encountering a NaN for example.

@fingolfin fingolfin deleted the mh/float-inf branch March 25, 2018 11:16
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants