Improved documentation, moved to float.gd, added IsRealFloat filter, …

[laurentbartholdi]

Improved float documentation and interface

This commit overhauls the documentation of the floateans in GAP, and adds and removes some commands from the interface:

• ComplexI is removed, it can be replaced by "MakeFloat(filter, 0, 1)"
• Overlaps is removed, it can be replaced by "not IsDisjoint(interval1, interval1)"
• filters IsRealFloat, IsComplexFloat, IsFloatInterval are used more systematically
• SignFloat, which does not permit to distinguish 0 and -0 and is not part of IEEE754, is supplemented by SignBit, which returns True if and only if its argument has the sign bit set
• the default method for Round is removed, because it can lead to precision loss
• RootsFloat, Cyc, and interval operations are now documented.

[codecov]

Codecov Report

Merging #2016 into master will decrease coverage by 13.78%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master    #2016       +/-   ##
===========================================
- Coverage      66%   52.21%   -13.79%     
===========================================
  Files         898      424      -474     
  Lines      273286   223086    -50200     
  Branches    12771    10418     -2353     
===========================================
- Hits       180372   116486    -63886     
- Misses      90087   103927    +13840     
+ Partials     2827     2673      -154

Impacted Files	Coverage Δ
lib/float.gd	45.45% <ø> (ø)	⬆️
lib/ieee754.g	91.3% <ø> (ø)	⬆️
lib/float.gi	28.51% <0%> (-3.14%)	⬇️
src/macfloat.c	61.9% <0%> (-13.92%)	⬇️
lib/attr.gi	0% <0%> (-100%)	⬇️
lib/teachm2.g	0% <0%> (-100%)	⬇️
lib/contfrac.gi	0% <0%> (-89.48%)	⬇️
lib/wpobj.gi	8.69% <0%> (-89.14%)	⬇️
grp/imf.gi	0% <0%> (-84.96%)	⬇️
lib/proto.gi	0% <0%> (-83.34%)	⬇️
... and 785 more

[fingolfin]

Could you please amend the commit message to follow our general guidelines, i.e.: first a single line summary with ideally only 50-60 chars; then an empty line, then a full description of the commit.

E.g.:

Improve float documentation and interface

This commit does:
* introduce IsRealFloat and IsComplexFloat
* remove Overlaps and ...
* ...

This way, when bisecting regressions in a year from now, one does not have to wonder whether this commit did certain things on purpose or by accident.

[fingolfin]

This is still incorrect for negative zero.

[fingolfin]

When you say "standard math library", what you mean? The C math library? Or maybe the POSIX math library? Or the linux (glibc) math library? Or something else?

These are all different things. And for me (on OS X), several of the functions listed above have either a different name in man 3 math (ignore case differences), e.g. CubeRoot(vscbrt), Square(vs.sqrt), Hypothenuse(vs.hypot), or they are missing altogether, e.g. Zeta,Frac, SinCos, Gamma`

So I would be very hesitant to mention man 3 math here, at least like this. I really think there should be one mansection for each of the above, with a description of what it does, and possibly the name of the corresponding function in a specific standard, such as ISO C, and/or perhaps references to Clause 9 of IEEE 754.

[fingolfin]

So ComplexI is effectively removed, but (a) this is not visible in the commit message, and (b) why is this commented out left in at all?

[fingolfin]

Why is this commented out line here?

[fingolfin]

I don't understand this: If a float implementation implements them better, then it would install its methods for its own filter, which would be a subfilter of IsRealFloat or IsComplexFloat, and hence would automatically have a higher rank.

Or perhaps you are concerned about float implementations which implement neither IsRealFloatnorIsComplexFloat(such asfloatright now, asIsRealFloatorIsComplexFloatdo not yet exist onmaster`) ? But then this should be said so explicitly

[fingolfin]

So Overlaps is removed, too, but this isn't noted in the commit message either.

[olexandr-konovalov]

@laurentbartholdi also you need to look at the Travis test which builds GAP manuals - it does not detect all problems well enough - i.e. it fails if the manual build failed dramatically, but passes if it has unresolved or multiply defined references. The log at https://travis-ci.org/gap-system/gap/jobs/314787107 shows that there are two multiply defined references: to AbsoluteValue and Norm.

[laurentbartholdi]

Dear @fingolfin: Thanks for your thorough review of my code! I mainly agree with all you suggest, but here are some further comments:

+InstallMethod( SignFloat, "for real floats", [ IsRealFloat ], -1, function ( x ) if x < Zero(x) then return -1; elif IsZero(x) then return 0; else return 1; fi; This is still incorrect for negative zero.

Hmmm... there's no standard "SignFloat" in IEEE754, as far as I see; however, there's a sign bit. I wanted SignFloat to be the same as SignInt, but for floats; that's maybe not so useful. On the other hand, I would be happy to scrap "SignFloat" which anyways is ambiguous on what it does to negative zero, and have SignBit with Float->Bool signature. What do you think?

+## <Description> +## These are all standard math functions, directly copied from the standard math library; see <C>man 3 math</C> for their list. When you say "standard math library", what you mean? The C math library? Or maybe the POSIX math library? Or the linux (glibc) math library? Or something else?

Yes, I looked them up in /usr/local/math.h and gave them more GAPish names. sincos exists on Linux, it's called __sincos on Mac OSX.

+#DeclareAttribute("ComplexI", IsFloat); #obsolete So ComplexI is effectively removed, but (a) this is not visible in the commit message, and (b) why is this commented out left in at all?

I will amend the commit message. I believe it should be removed, but I wanted to keep a trace of that decision. I understand it's also part of the git history, so one could remove the commented line.

+DeclareAttribute("Mid", IsFloatInterval); +DeclareAttribute("AbsoluteDiameter", IsFloatInterval); +DeclareAttribute("RelativeDiameter", IsFloatInterval); +#DeclareOperation("Diameter", IsFloat); Why is this commented out line here?

I'd rather leave this one in: I really want to declare an operation "Diameter", synonym of "AbsoluteDiameter" which is the most common one. There was a clash with other packages declaring Diameter, so temporarily I would leave it out before the interval interface is settled.

In lib/float.gi <#2016 (comment)>: > @@ -252,23 +252,21 @@ end); ############################################################################# ## Default methods +## +## These methods have priority -1, because they are inefficient. +## Hopefully every float implementation implements them better. I don't understand this: If a float implementation implements them better, then it would install its methods for its own filter, which would be a subfilter of IsRealFloat or IsComplexFloat, and hence would automatically have a higher rank. Or perhaps you are concerned about float implementations which implement neither IsRealFloatnorIsComplexFloat(such asfloatright now, asIsRealFloat orIsComplexFloatdo not yet exist onmaster`) ? But then this should be said so explicitly

Yes, I'd rather make sure that the method only kicks in as a last resort. I'll add a comment.

[fingolfin]

Re SignFloat: well, there is signbit in the C99 standard. Also, copysign. Both of these agree that -0.0 has sign -1.

Of course SignFloat (like all other functions) is undocumented, so I cannot really argue that it is wrong. After all, it's (unwritten) documentation could state: "SignFloat always returns the string "Hello!".` -- which goes to say that I'd really like us to explicitly document all these functions (but not necessarily via this PR, of course).

Re: math.h and man 3 math: Well, what you say regarding sincos I think fully confirms what I say regarding that this is not a "standard math library" in any good sense. Hence my objection to that statement in this PR.

Re "leaving a trace on the decision to remove ComplexI": First, as you say yourself, this is what the git history is for. Second, even if one argues that this is not enough, I'd argue that leaving an commented out line declaring ComplexI is not helpful to anybody: It just raises the questions "why is this line there?" or "why is this commented out?", but explains nothing. So to answer this, people need to look at the git history, or ask. I.e. leaving this is only useful if there is also a comment next to it, saying something like "GAP used to declare the attribute ComplexI up to version X.Y, but it was removed in GAP Z. Use yadda-yadd instead.".

Re: Diameter: Again, I think leaving in a commented out line like that is useless for your intent. If you want to use Diameter as a synonym, open an issue stating that goal, and let's work towards it. Or just do it: Diameter is an operation in grape. Other packages also use it. So I think it'll have to stay an operation. But of course you can declare it as an operation applicable to floats, too, and install a single method to it, which dispatches to AbsoluteDiameter. Of course you then can't use HasDiameter and SetDiameter (although I guess you could provide operations with that name, too...), but still users can invoke it. (Of course then it should also be documented, with the documentation stating that it is an alias for AbsoluteDiameter)

[olexandr-konovalov]

Manual building log looks OK now (the unresolved references are because of the links to Example package from doc/dev manual - I will fix that). @fingolfin are you happy to merge this? If so, this is certainly an improvement, worth to put in 4.9.0.

[fingolfin]

@alex-konovalov I don't have time to review this again, but I don't object. Right now, it's your review that is blocking a merge, though.

[olexandr-konovalov]

@laurentbartholdi it seems that this now requires a new release of float, isn't it?

[laurentbartholdi]

Deal @alex: sorry, I forgot to mention this! Yes, I had bundled float 0.9.0 with the requirement "gap >= 4.9"; I hope that's the good way to do it.

[olexandr-konovalov]

@laurentbartholdi ok, now after this change has been merged, float 0.9.0 works fine.