Nemocas / Nemo.jl

Julia bindings for various mathematical libraries (including flint2)
http://nemocas.github.io/Nemo.jl/
Other
176 stars 57 forks source link

Drop redundant check_parent methods #1784

Closed fingolfin closed 3 weeks ago

fingolfin commented 4 weeks ago

... in favor of generic one from AA 0.41.9

Some removed methods are for matrices, and didn't check the parent, but instead the base ring, ncols, nrows -- but they should be fine performance-wise because the check_parent(a::MatrixElem, b::MatrixElem, throw::Bool = true) method from AA should kick in which does just that.

Alas in practice it seems something is still broken, tests fail, so I assume I screwed something up. But I am posting this as a draft here now so I won't forget about it.

thofma commented 4 weeks ago

@joschmitt can you have a look? it is failing in the solving

joschmitt commented 4 weeks ago

@joschmitt can you have a look? it is failing in the solving

Apparently, I assumed that check_parent for matrices only checks whether the base rings coincide. This didn't hurt so far because the check_parent methods for finite fields only complain if the number of rows AND columns differ but not if only one of them differs. I assume that this is a mistake.

I will open a pull request (#1785) that makes the correct checks in the linear solving methods and then you can rebase this or cherry-pick.

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.72%. Comparing base (8aedf5e) to head (8201899). Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1784 +/- ## ========================================== - Coverage 85.90% 85.72% -0.19% ========================================== Files 95 95 Lines 36491 36355 -136 ========================================== - Hits 31349 31164 -185 - Misses 5142 5191 +49 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lgoettgens commented 3 weeks ago

This is ready to go from my POV, but I cannot merge it due to required CI jobs not running. @fingolfin maybe you just do it yourself then?