Fix Order(K, [ ... ])

[joschmitt]

Closes #1223 .
I basically reimplemented the functionality now and all the errors in #1223 should be solved. In the examples collected from the tests by @thofma , this is always faster or at least comparable to the runtime of the old function.
I am not sure I take the best modulus for the modular HNF; maybe some guru could have a look.

[thofma]

Would be great if you could squash and add a comment explaining the algorithm, including the start thingy.

[fagu]

I don't have counterexamples, but this feels wrong.

1. You only apply Hermite reduction when you have at least n vectors: https://github.com/thofma/Hecke.jl/blob/401a7116869fc881c4ea339899aa271cc3da7a6a/src/NumFieldOrd/NfOrd/NfOrd.jl#L1052C1-L1052C1
   I didn't check, but from the name of the function, I would guess that is_zero_mod_hnf!(a,b) assumes that b is Hermite reduced. This is called by in_span_of_B even before you have n vectors.

2. The start logic doesn't make sense to me. It for example means that if at the beginning of step i (in which add multiples of e^i), you already had n vectors, you will not do anything in step i+1 because start=n+1 and length(bas)=n.

[fagu]

Sorry, never mind about the start thing. I missed a later line.

[fagu]

Ah, there's a difference between what's in B and what's in bas. That probably fixes my first concern. (Sorry about the noise...)

[joschmitt]

Yes, B and bas are not in sync (that's the same as before). B should always be in HNF; it starts with a single line.

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (41b61f3) 74.59% compared to head (03dd36d) 74.53%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
- Coverage   74.59%   74.53%   -0.06%     
==========================================
  Files         346      346              
  Lines      110839   110840       +1     
==========================================
- Hits        82681    82618      -63     
- Misses      28158    28222      +64     

Files	Coverage Δ
src/Misc/Matrix.jl	75.75% <100.00%> (+0.25%)	⬆️
src/NumFieldOrd/NfOrd/NfOrd.jl	87.12% <100.00%> (+2.13%)	⬆️

... and 30 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thofma]

@fagu thanks for having a look.

@joschmitt Can you add a test case for the line with missing coverage?

@fieker any comments?

[fieker]

I have not tried to check the math, but it looks fine to me. Do you happen to recall the examples that sparked the extends stuff? My guess would have been a large degree normal field where extends was applied with the equation order and either some subfield generator or an automorphism...
That would result in some interesting test cases to benchmark (I don't think this is worse than the old stuff)

[fagu]

The algorithm looks correct to me now. Thanks for the fixes and clarifications!

Lines 1016-1018 and 1084-1086 seem unnecessary, but maybe I'm wrong.

Small comments / questions on the modular HNF:

• Any invertible matrix in Hermite normal form is triangular, so computing the determinant is easy. Does the det function recognize this?
• Whenever you save the determinant of the numerator (m=...), you could also save the denominator, and then later use modulus (current denominator)*(earlier determinant)/(earlier denominator), I think.
  The point is that (determinant)/(denominator) * Z^n is contained in the Z-module spanned by the rows of B.
• Maybe also update the determinant of the numerator and the denominator after line 1072 if (new determinant)/(new denominator) < (old determinant)/(old denominator)?

[fagu]

Doesn't check = false or check = (check && extends !== nothing) suffice, depending on whether or not we're supposed to check that extends is actually an order?

You've already checked that the generators are integral.

The Order constructor with check = true reduces all products of pairs of basis elements modulo the (Hermite reduced) basis. For example if there is only one generator, this seems like an unnecessary amount of work.

[joschmitt]

This depends on the general philosophy, I think. My understanding so far is that we by default rather check correctness one time too often, but let the user turn off the checks, if they know what they are doing.

[joschmitt]

@joschmitt Can you add a test case for the line with missing coverage?

I added the test.

[joschmitt]

I have not tried to check the math, but it looks fine to me. Do you happen to recall the examples that sparked the extends stuff? My guess would have been a large degree normal field where extends was applied with the equation order and either some subfield generator or an automorphism... That would result in some interesting test cases to benchmark (I don't think this is worse than the old stuff)

A real life example would be good. In the tiny examples I had from the tests also the modular approach never really made sense.

[joschmitt]

Lines 1016-1018 and 1084-1086 seem unnecessary, but maybe I'm wrong.

Lines 1016-1018 are exactly to catch the case that somebody submits an empty array like in #1223 Issue 2.
Lines 1084-1086 are 'unnessary', but don't hurt in my opinion. They certainly helped with debugging.

Small comments / questions on the modular HNF:

* Any invertible matrix in Hermite normal form is triangular, so computing the determinant is easy. Does the `det` function recognize this?

I do not know.

* Whenever you save the determinant of the numerator (`m=...`), you could also save the denominator, and then later use modulus (current denominator)*(earlier determinant)/(earlier denominator), I think.
  The point is that (determinant)/(denominator) * Z^n is contained in the Z-module spanned by the rows of B.

This modular business is always confusing to me when 'fractional types' are involved. My understanding is the following: I have the FakeFmpqMat B given by an integer matrix num(B) and a (integer) denominator den(B). Now I know another FakeFmpqMat M such that the $\mathbb Z$-module spanned by M is contained in the one spanned by B and I want to get the HNF of B by computing modulo M. The function hnf_modular_eldiv takes a multiple of the largest elementary divisor of the matrix as modulus. It is not documented for FakeFmpqMat, but from looking at the implementation, I assume that I need to choose this modulus with respect to num(B). So I have the inclusion den(B)*num(M)/den(M) \subseteq num(B) and it seems to me that the best possible guess for the largest elementary divisor of num(B) is den(B)*(largest elementary divisor of num(M)) from this. And my best guess for the largest elementary divisor of num(M) is unfortunately det(num(M)).

* Maybe also update the determinant of the numerator and the denominator after line 1072 if (new determinant)/(new denominator) < (old determinant)/(old denominator)?

It is not clear to me whether the computation of the determinants (in practise) is worth the effort of finding a (slightly) better modulus. Here a real world example might help.

[joschmitt]

I added a specialized determinant for triangular matrices and tuned the modular stuff a bit as discussed with @thofma .

For me, this is good to go (assuming CI agrees). If anybody needs it to be faster, I would need a real-life example.