Make gcdx(0, 0) return (0, 0, 0)

[albinahlback]

As the title suggests. This should conform to GMP's definition: https://gmplib.org/manual/Number-Theoretic-Functions#index-Extended-GCD.

[Keno]

Is there a justification for why one is better than the other? This is a silent breaking change, so we cannot do this lightly.

[albinahlback]

Sure, I understand. Basically it is because of (1) standards, (2) expectation and (3) bad practice (IMO).

As mentioned it is the standard for GMP, but also for other programmes like Wolfram: https://www.wolframalpha.com/input/?i=extendedgcd%280%2C0%29.

When I write gcdx(a, b) I want an output (d, x, y) such that x a + y b = d. For the function to be well-defined, I think I speak for everyone that we want the "smallest" x and y (for a more precise specification of what smallest mean, see https://fungrim.org/entry/da7d00/ and https://fungrim.org/entry/633265/). Just as it would be wierd to solve 14 x + 12 y = 2 by setting (x, y) = (-11, 13) when (x, y) = (1, -1) is the "natural" solution, it would be just as wierd to solve 0 x + 0 y = 0 by setting x and y to something non-zero.

Continuing on this, I find it bad practice to not return the "smallest" values. If we can return a small number, why don't we?

Obviously this need to be documented, but the general behavior of gcdx should be defined as well.

[oscardssmith]

This makes sense to me. Adding triage to confirm. @Keno why is this a breaking change? I think as long as the result conforms with the definition, it's OK.

[albinahlback]

This makes sense to me. Adding triage to confirm. @Keno why is this a breaking change? I think as long as the result conforms with the definition, it's OK.

Well, it is a breaking change as people may rely on specific return values. However, I still think it is good to proceed.

[LilithHafner]

@nanosoldier runtests()

[oscardssmith]

Triage approves on the argument that this is a pretty minor change, arguably more mathematically consistent, and agrees with GMP and Mathematica.

[nanosoldier]

Your job failed. Consult the server logs for more details (cc @maleadt).

[maleadt]

Looks like an unrelated compilation failure (SuiteSparse checksum failure); let's try again:

@nanosoldier runtests()

[nanosoldier]

Your job failed. Consult the server logs for more details (cc @maleadt).

[maleadt]

... wait, the head of this PR is ancient. Let's rebase first.

[maleadt]

@nanosoldier runtests()

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[oscardssmith]

lol. the one failure is Nemo which 5 years ago discovered that Nemo disagreed with julia and therefore in Nemocas/Nemo.jl#730 switched how their gcdx works. Specifically, see https://github.com/Nemocas/Nemo.jl/blob/a5a9c5c2d565a94ad8d75ed484848d2d60d19cb2/src/flint/fmpz.jl#L1282.

[vtjnash]

Is this related to / close #39638?

[LilithHafner]

no

[albinahlback]

Can I bump this?

[LilithHafner]

Thanks for the bump.

This should get an entry in NEWS.md (follow the style of the other entries in that file)

Aside from that LGTM.

It would be nice to test gcdx on BigInt.

[albinahlback]

Haven't checked that it passes the tests. I also added some extra tests for gcdx and added tests for BigInt. I did not do anything more with rationals, though. Added an entry in NEWS.md.

[LilithHafner]

Elegant approach to testing. I like it!

[LilithHafner]

Thank you for your contribution!