small change in rational constructor

[Jutho]

This small change is required for a custom type of mine that represents integers as their prime factorization. In this format, *, lcd and gcd are easy, and so is divgcd. However, general div is not necessarily easy.

This PR also fixes consistency. On master divgcd is used in the various // methods, but not in the actual Rational constructor itself.

[StefanKarpinski]

Seems reasonable enough to me. Anyone have any objections to this change? I guess my one concern is checking the sign of the denominator. But I suppose if one uses a type where sign doesn't make sense, it can always just return 1.

[tkelman]

should probably @nanosoldier runbenchmarks(ALL, vs=":master") just in case, though not sure how thoroughly rationals are covered

[Jutho]

Thanks for the review. The sign part was something I added in the second commit, since my PrimeFactorization type does not even support comparing two arbitrary numbers easily (i.e. without converting to another format). However, getting the sign is easy I could certainly just write a method for < that deals with the comparison to zero. There are probably other cases where sign doesn't make sense, so I could undo this second commit.

[StefanKarpinski]

If den < 0 works then sign(den) probably should as well, so this seems fine. Perhaps the divgcd function should include the normalization so that the first value is always positive when that makes sense? That way each type can define this in a way that works for it.

Edit: although I guess that means that divgcd would no longer just return the arguments divided by the gcd – once upon a time the gcd function always returned a gcd with sign matching the first argument, but I guess someone didn't like that behavior.

[Jutho]

divgcd does not seem to be used in base outside of rational.jl, so I can change that if you like (though that behaviour is not explicitly implied by the name of the function).

I am fine with any suggestion that gives me a handle on modifying the construction of Rational{MyType}. Since Rational{T} uses an inner constructor, it did not seem possible to modify its behaviour for my particular T=MyType other than creating this PR.

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[Jutho]

Those benchmark results seem unbelievable; is everything running correctly? If so, than I will have to check what is going on, but most of these regressions seem completely independent of Rational at first sight.

[fredrikekre]

Likely some noise? Let's rerun
@nanosoldier runbenchmarks(ALL, vs=":master")

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[Jutho]

All regressions from latest benchmark seem to fall just outside of the tolerance window, so I guess it is still noise?

@StefanKarpinski , would you like me to change divgcd as you proposed, or are you fine with the current implementation?

[StefanKarpinski]

I'm cool with your current PR. If it comes up in the future, we can always introduce a new factorization point for this and I think divgcd's current behavior is what it should do.

[Jutho]

Ok great. And you agree with my assessment that the benchmark regressions are noise? Is CI always so unreliable these days? Any other requests/requirements before this can be merged?

PS: This is what motivated this PR -> https://github.com/Jutho/WignerSymbols.jl

[StefanKarpinski]

Regressions are probably noise.

Is CI always so unreliable these days?

Yes. @travis-ci has been oversubscribing their machines for months now and it has made their service (which we pay for) completely useless since it never actually passes. We have raised the issue with them multiple times and so far they have not managed to do anything helpful. @appveyor is not usually too bad – they should consider offering Linux and/or Mac CI to take advantage of the situation and get some new customers. We would happily jump ship from Travis at this point, and we are, in fact actively investigating switching to @circleci, which seems to have much better performance (#23188).

[fredrikekre]

Lets rerun CI, but should be good to go?

[Jutho]

Thanks