Skip to content

Total ordering for floating-point values.#3022

Merged
DmitryOlshansky merged 15 commits intodlang:masterfrom
ivan-timokhin:totalOrder
Sep 2, 2015
Merged

Total ordering for floating-point values.#3022
DmitryOlshansky merged 15 commits intodlang:masterfrom
ivan-timokhin:totalOrder

Conversation

@ivan-timokhin
Copy link
Contributor

Adds a totalOrder function as specified in IEEE 754-2008.

An immediate benefit is the ability to sort arrays of floating-point values and get predictable results even in presence of NaNs (see http://dpaste.dzfl.pl/1f78bc6f7d2b).

@schuetzm
Copy link
Contributor

Maybe return -1, 0 and 1? Then it can be directly used in opCmp().

@Orvid
Copy link
Contributor

Orvid commented Feb 23, 2015

I agree, returning -1, 0, and 1 would be significantly more useful than returning a bool.

@ivan-timokhin
Copy link
Contributor Author

I tried to follow semantics described in the standard, and it explicitly specifies the return type as boolean. I'm not sure how important that is, but that was the motivation for the current design.
Besides, the three-way comparison would still be unusuable in e.g. sort, because it requires a predicate (and current version is unusable because the order isn't strict).

It seems like it would be nice to have 3 functions:

  1. Something returning -1, 0, and 1 for opCmp.
  2. Strict comparison predicate for sort and friends.
  3. Current function for strict IEEE compliance.

However, having all three is definitely an overkill. So, which one?

@Orvid
Copy link
Contributor

Orvid commented Feb 24, 2015

I don't see that being overkill in the slightest.

@ivan-timokhin
Copy link
Contributor Author

I decided to settle on option 1, because

  • the problem with sort isn't specific to this particular case — the same problem exists with e.g. std.algorithm.comparison.cmp;
  • strict IEEE compliance isn't really that important, especially since standard behaviour doesn't seem to fit any of use-cases directly.

And I still don't think it's a good idea to have three functions doing the same thing in slightly different ways.

@CyberShadow
Copy link
Member

Code style nit, we generally don't put braces around single statements in Phobos.

std/math.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

So, looking back at this, I have no idea how I failed to notice this. I would suggest an if-else if chain rather than this, as it is much clearer with no difference in performance.

@ivan-timokhin
Copy link
Contributor Author

Fixed.

@DmitryOlshansky
Copy link
Member

@9il you look like a numerics expert :) Would you mind reviewing this? Thanks!

@9il
Copy link
Member

9il commented Jul 15, 2015

I'll try:) Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The last else block should always be a static assert.

Here should instead be else static if (F.realFormat == RealFormat.ibmExtended)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std/math.d Outdated
Copy link
Member

Choose a reason for hiding this comment

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

unroll please

@9il
Copy link
Member

9il commented Jul 16, 2015

sort function has note about floating point. Please add reference to the new function into the sort docs.

@9il
Copy link
Member

9il commented Jul 16, 2015

... and one documented unittest for sort!((x, y) => cmp(x, y) < 0) into the sort docs too.

@9il
Copy link
Member

9il commented Jul 16, 2015

If this function works faster then naive floating point comparison then we can make it default for functions like sort.

std/math.d Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: empty line looks unnecessary here.

@ivan-timokhin
Copy link
Contributor Author

ivan-timokhin commented Jul 16, 2015 via email

@ivan-timokhin
Copy link
Contributor Author

ivan-timokhin commented Jul 16, 2015 via email

@9il
Copy link
Member

9il commented Jul 16, 2015

Hm. I used $(XREF) for cross-module referencing, but the resulting link points
to stable docs on dlang.org, which, of course, contain no such function. What
should I do?

Nothing. XREF is fine.

@9il
Copy link
Member

9il commented Jul 16, 2015

LGTM except name cmp. fcmp would be more clear, thought.
Ping @DmitryOlshansky

@ivan-timokhin
Copy link
Contributor Author

I dunno. The first thing fcmp reminds me of is the whole C math functions family with names like fabsf and atanl, which exist because of lack of overloading.
However, I'm not attached to any particular name, so I'll happily go with fcmp, provided there's consensus.

@jpf91
Copy link
Contributor

jpf91 commented Jul 16, 2015

OT: As we seem to have most D math experts assembled in this thread:

Could somebody have a look at this issue:
https://issues.dlang.org/show_bug.cgi?id=13032

AFAICS std.math.internal.gammafunction is the last module with only works with X86 80bit reals and has never been ported for standard double / float formats. As this probably affects every non X86 architecture it'd be great if somebody could have a look.

@9il
Copy link
Member

9il commented Jul 20, 2015

@jpf91 Hi, std.mathspecial was ported from CEPHES library (thought). You may want to ask @donc about license change and port stuff for float and double versions.

@jpf91
Copy link
Contributor

jpf91 commented Jul 20, 2015

Thanks, I had a quick look at gammafunction.d/CEPHES. The constants have been renamed in the D port, the code layout is different and the constant values also don't match the constants in gammal.c. As I don't have any experience with numerical maths porting gammafunction.d for double types is more like random-guessing for me ;-)

@ibuclaw
Copy link
Member

ibuclaw commented Jul 20, 2015

FYI see std.math for the license of redistributing CEPHES in another language (I spoke to the original author for clarification a couple years ago).

@9il
Copy link
Member

9il commented Jul 20, 2015

Porting is pretty simple except constants. Sometimes they are defined in different binary forms (depends on endianness). The steps can be described as:

  1. Port with core.stdc.math or core.stdc.tgmath
  2. Add unittests. Floating hexadecimal approximate constants for unittests can be computed with Wolfram Alpha.
  3. Port with std.math.

@9il
Copy link
Member

9il commented Jul 20, 2015

@jpf91 See also #3045

@ibuclaw
Copy link
Member

ibuclaw commented Jul 20, 2015

Floating hexadecimal approximate constants for unittests can be computed with Wolfram Alpha.

I'm not familiar with Wolfram, I'd be interested to know how you do this.

@9il
Copy link
Member

9il commented Jul 20, 2015

@ibuclaw Example: hexadecimal log(2^52).

@9il
Copy link
Member

9il commented Jul 20, 2015

Also you can press More digits.

@DmitryOlshansky
Copy link
Member

LGTM except name cmp. fcmp would be more clear, thought.
Ping @DmitryOlshansky

Okay I'm here. Press the button I guess? :)

@9il
Copy link
Member

9il commented Sep 2, 2015

Yes please =)

@DmitryOlshansky DmitryOlshansky added this to the 2.069 milestone Sep 2, 2015
@DmitryOlshansky
Copy link
Member

Yes please =)

I've added milestone but @ivan-timokhin please consider adding a nice changelog entry or the change may go unnoticed.

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

DmitryOlshansky added a commit that referenced this pull request Sep 2, 2015
Total ordering for floating-point values.
@DmitryOlshansky DmitryOlshansky merged commit f2220c2 into dlang:master Sep 2, 2015
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.

9 participants