Solve

[a-kulkarn]

This is what I've been up to over the last week. The changes build off the last PR. This PR is mostly for information, and is still missing features. New additions include:

++ More tests!
++ hnf_solve
++ top level solve dispatch
++ Solve left/right for solve_lu, solve_hnf, solve_fflu.
++ Rectangular solving for all three systems.
-- Merged the triangular back-solve logic for solve_lu, solve_hnf, solve_fflu into one function.
-- removed separate/duplicate functions for left/right logic. Now you just pass a parameter.

Almost all tests pass. I've noticed a few anomalies so far:

1. The assignment logic goes a little wrong when the principal rxr minor is not the invertible one. Fixing this properly requires extensions to the "view" function.
2. Solve_rational over a non-domain seems to fail the consistency check often.
3. Noticed that fflu! behaves weirdly in rectangular, rank-deficient cases. Some maintenance is required.

Next up is adding
A. A lower triangular solve. (It is technically possible to avoid this with indexing tricks, but that's more complicated than a second function).
B. Fix (1.)
C. With (A), implement the kernel/nullspace=true options.
D. More tests. always.

[wbhart]

I'll need to take a closer look, but a quick look through and I think this looks great. I have just a few trivial comments so far:

• Obviously we need to sort out the broken tests before merging
• I prefer not to leave commented out code. It's in the Git history if you need it. It just makes it harder to find the working code (e.g. with search and replace)
• There are still quite a few places where == and + don't have spaces before and after. The only exception to this is keyword arguments, where we don't usually put spaces around the =.
• I believe it is not necessary to use === when comparing with a symbol. These are "interned" and ordinary comparison is just as cheap as ===. That is the major benefit of symbols over strings, I believe.

There will be more comments when I take a closer look.

We will also want to check timings of past benchmarks to make sure nothing has slowed down. There are probably some on the Nemo website nemocas.org, though some of those are using Flint matrices. Only the generic ones are relevant.

[wbhart]

I have a few more comments from another brief look:

• I see that Matrix.jl has been broken up into files. These names should all begin with Matrix. So LU-variants.jl should be MatrixLU.jl, so that it is easy to identify which files are from the original Matrix.jl.

• The Matrix-auf-Hecke will have to be cleaned up and distributed amongst other files before it can be merged. We generally don't accept incomplete code in AbstractAlgebra.jl as it is a very mature library and obviously accumulating more immature code does not converge.

• There is currently already a divexact_left defined in NCRings.jl which already works for Rings, so there is no need to introduce this. And in any case, Rings.jl would be the wrong place to do this. NCRings.jl would be the right place.

[a-kulkarn]

Thanks for the response! I'm going to push in some more changes from my latest batch. Indeed, I agree that much maintenance needs to be done before a merge can occur. That said, I think I need to somehow make the code visible to the AA community :-). Here are some responses:

> I see that Matrix.jl has been broken up into files.

Agreed

> The Matrix-auf-Hecke will have to be cleaned up and distributed amongst

other files before it can be merged. Also agreed. However, it is important to remember this is a manifest for what some people actually want. It's certainly worth the time to polish, but I can't do it alone.

> There is currently already a divexact_left defined in NCRings.jl which

already works for Rings. About that... This assumes that `divexact_left` is implemented for the concrete type. Otherwise, if you apply it to a ring type, it results in an infinite loop. It is correct to have divexact_left implemented for commutative rings be equivalent to a `divexact` call, and at least for non-commutative rings that embed in a skew-field, divexact is equivalent to divexact_left if and only if the ring is commutative. This will allow commutative rings to hook into anything we do for non-commutative rings automatically without adding a burden to the "required interface". Best,

…

On Tue, Dec 10, 2019 at 4:21 PM wbhart ***@***.***> wrote: I have a few more comments from another brief look: - I see that Matrix.jl has been broken up into files. These names should all begin with Matrix. So LU-variants.jl should be MatrixLU.jl, so that it is easy to identify which files are from the original Matrix.jl. - The Matrix-auf-Hecke will have to be cleaned up and distributed amongst other files before it can be merged. We generally don't accept incomplete code in AbstractAlgebra.jl as it is a very mature library and obviously accumulating more immature code does not converge. - There is currently already a divexact_left defined in NCRings.jl which already works for Rings, so there is no need to introduce this. And in any case, Rings.jl would be the wrong place to do this. NCRings.jl would be the right place. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#559?email_source=notifications&email_token=AHEEZKKRAB76QTZL744S46DQX6XWTA5CNFSM4JYASVE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGPTPDA#issuecomment-564082572>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHEEZKJLHOQPV5JGBUHIXFLQX6XWTANCNFSM4JYASVEQ> .

[a-kulkarn]

New updates from the previous two days:
A. A lower triangular solve added.
B. (1.) fixed
D. More tests. always.

Sadly, kernels/nullspaces have still not been implemented. All AbstractAlgebra tests pass, as well as the new ones I've created. I am unfortunately out of productive time in the short term, so someone else is likely needed to carry the torch.

Next steps:
-- Add the new FFLU variant. (a buggy prototype is pushed to a branch on my fork, based on the work wbhart and I did earlier today, but not debugged at all)
-- kernels/nullspaces
-- Nemo compatibility (might require some patching to Nemo)

[wbhart]

I think this will be a long ongoing work that will take some time to sort out.

I guess Tommy or I will eventually finish this project off.

I'm currently busy until about the end of August. But I guess at some point after that it might be possible to work on this, if not before. In the mean time, we'll just rebase it a few times to keep up with new AbstractAlgebra releases (if any).

[wbhart]

I have added it to the list of important things to be done for Oscar:

oscar-system/Oscar.jl#1

[a-kulkarn]

Great, thanks! Hope this helps...

…

On Tue, Dec 10, 2019 at 5:36 PM wbhart ***@***.***> wrote: I have added it to the list of important things to be done for Oscar: oscar-system/Oscar.jl#1 <oscar-system/Oscar.jl#1> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#559?email_source=notifications&email_token=AHEEZKMXGCVWSMHZAQDDGC3QX7AQXA5CNFSM4JYASVE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGP4LOI#issuecomment-564118969>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHEEZKNZATFDMKLI7MJBLRTQX7AQXANCNFSM4JYASVEQ> .