-
-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MeatAxe-related bug introduced in 8.3.beta #25476
Comments
comment:1
Practical problem: I can currently not build and test 8.3.beta3, since I won't interrupt a computation that I am currently running (it is running since three weeks and might disprove a conjecture on the structure of cohomology rings). |
comment:2
This also occurs in testing |
comment:3
So this seems to be caused by #17272, but I do not understand why. It is the exact same code for the classical echelon algorithm. The only other difference is that has become a Also, failing tests with optional packages is a blocker. |
comment:4
PS - I think the machine I tested #17272 on did not have |
comment:5
I really have no idea what is going on. It does work before #17272, but fails after. Changing both the |
comment:6
While trying to build the latest beta, cddlib fails. Very bad. So, I still cannot test stuff. |
comment:7
Fortunately retrying |
comment:8
Now I completed building the latest beta and can confirm the bug. |
comment:9
PS: It is not the creation of the matrix but calling |
comment:10
OK, the problem is that the algorithm has been changed so that only a part of a row was rescaled. That's quite reasonable to do, and actually one of the changes of SharedMeatAxe compared to MeatAxe is that in the school book gaußian elimination implementation I made it so that the already annulated part of a row is not rescaled. Only in the wrapper in Sage I did not expose that functionality. I guess that needs to change. |
comment:11
That part of Gaußian elimination was not changed by #17272. Ah, I found the problem: the method was renamed |
comment:12
I will push a fix in 1 second. |
Commit: |
comment:13
Okay, so this fixes the problem. It has a bit of technical debt as it pushes a potentially bigger issue design issue down the road, but it works. I also do similar fixes for other matrices that may not be using their optimized code. A bigger design discussion on matrices is likely needed. New commits:
|
comment:14
So, you think it won't be needed to allow !Matrix_gfpn_dense to perform a partial row rescaling? Would it nonetheless be a good idea to implement? |
comment:15
I definitely think it would be good to do partial row rescaling; it probably will give you a speed boost. Although that this actually indicated an error turned out to be a good thing indicating something I missed when reviewing #17272. |
comment:16
Replying to @tscrim:
Then I propose to provide an enhancement on top of your changes. I just checked: With your branch the tests in src/sage/matrix pass. And then I hope we can review each other's commits. |
comment:17
Replying to @simon-king-jena:
In principle, I would fine with it. However, given this is a blocker issue, I think this would be better to do so on a follow-up (which I will happily review). |
comment:18
So far I got a timeout in |
comment:19
Replying to @simon-king-jena:
That has been noticed by a few other people. |
comment:20
I cannot imagine that it is related with the issue we are dealing with here - hence, I'll try if these two tests fail without the patch from here. |
comment:21
Yep, I get the same timeouts. So, I believe the timeouts are not related with this ticket and thus shouldn't prevent a review. The code looks good. But shouldn't there be a test that demonstrates that the bug is fixed? Or are the existing tests good enough? |
comment:26
Replying to @vbraun:
The patchbot complains that there is a non-ascii character. But I cannot identify it. So, what is it? The patchbot complains about pyflakes. But the error is "NameError: name 'isPythonFile' is not defined". Doesn't that mean there is a bug in the plugin, not in the changeset? The patchbot complains about startup time. I don't know how to interpret the numbers, so I don't know how much of a problem we have here. The test error is reproducible as follows:
Hence, |
comment:27
The generic implementation's documentation does not state what return value it gives. However, it apparently returns the pivots. So, I guess I shall modify the _echelon_in_place_classical method of Matrix_gfpn_dense. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Changed author from Travis Scrimshaw to Travis Scrimshaw, Simon King |
comment:29
Done! Should work now, regardless whether meataxe is installed or not. Hopefully someone can review the last commit. |
comment:30
Ouch. |
comment:31
But these failures seem all to be "optional: internet". Anyway, I got them with |
comment:32
PS: I'll try again with a clean develop branch. |
comment:33
Loads of errors, but at least the last few errors I saw are due to what was fixed in this ticket. Frustrating. |
comment:34
Replying to @simon-king-jena:
The "optional - internet" tests are failing, too. |
comment:35
Replying to @simon-king-jena:
By "frustrating" I mean the fact that a clean develop branch doesn't pass tests. So, how could one possibly do development without a proper base to start with. |
comment:36
The internet tests failing are in some sense recent: Sage is supposed to automatically test for an internet connection and then run those tests, but the function |
comment:37
You can also run the tests manually with |
comment:38
Replying to @simon-king-jena:
Don't use |
comment:39
Replying to @simon-king-jena:
Travis, can you do that? This is making it very hard to work on any matrix-related ticket (like #23719, #25079 and #25504) |
comment:40
LGTM. Thank you. |
Changed reviewer from Simon King to Simon King, Travis Scrimshaw |
Changed branch from public/linear_algebras/fix_meataxe_echelon-25476 to |
Vincent reports this for SageMath 8.3.beta3 with MeatAxe installed:
The above error does not occur in 8.2.rc4. Let's try to prevent it from being published in 8.3!
CC: @videlec @tscrim
Component: packages: optional
Keywords: meataxe rescale_row
Author: Travis Scrimshaw, Simon King
Branch/Commit:
e2f0550
Reviewer: Simon King, Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/25476
The text was updated successfully, but these errors were encountered: