smash meataxe tweaks #1027
Merged
smash meataxe tweaks #1027
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fingolfin
added
the
kind: enhancement
fingolfin
force-pushed
the
mh/meataxe
branch
2 times, most recently
from
56cdfa1
to
daf1c96
Compare
Current coverage is 50.18% (diff: 100%)@@ master #1027 diff @@
==========================================
Files 424 424
Lines 223266 223096 -170
Methods 3430 3430
Messages 0 0
Branches 0 0
==========================================
+ Hits 110545 111959 +1414
+ Misses 112721 111137 -1584
Partials 0 0
|
|
With this PR, the coverage of Note that the new tests are not very clever or anything, they are just meant to ensure the code gets run through at least once, to catch "obvious" regressions. A comprhensive test suite would ideally test each documented meataxe API on its own (this could be facilitated by adding manual examples to each of them). It would also cover "tricky" cases, etc. But in the meantime, this is an improvement over what we had before. |
frankluebeck
reviewed
Jan 4, 2017
| @@ -3096,11 +3096,11 @@ local cf,dim,b,den,sub,i,s,q,found,qb; | |||
| Add(sub,TriangulizedMat(b)); | |||
| den:=dim; | |||
| else | |||
| TriangulizeMat(s); | |||
| s:=TriangulizedMat(s); | |||
There was a problem hiding this comment.
Isn't the old version better?
With qb below you changed the code the other way round.
There was a problem hiding this comment.
The problem is that s can be immutable (in fact, with this PR, it is always immutable), and then this function runs into an error. Hence we must make a copy.
|
Thanks for this cleanup! I have not done any test computations, but the code changes look fine to me. |
Previously, for trivial modules it wasn't
This gets rid of several unnecessary calls to ConvertToVectorRep
* Instead of passing matrices into a polynomial via Value, and then converting the result to an immutable matrix, it is better to convert the original matrix. This way, the polynomial evaluation is faster, and the resulting matrix is automatically immutable and in matrix rep. * Replace several loops computing linear combinations of matrices by invocations of Sum(). * Precompute (immutable) identity matrix once, instead of computing it again and again. * Replace ConvertToMatrixRep followed by MakeImmutable with a single call to ImmutableMatrix.
* ensure intermediate matrices are in matrep * don't recompute P^-1 when we already have it in Pinv * since centmat is immutable matrep, plugging it into a polynomial also yields an immutable matrep -> newcentmat is in matrep
Also, turn both "queues" into stacks, as this is more efficient (one of the two was a stack already anyway)
* create zero vector of length N+1 directly ... * use SortBy instead of Sort if suitable * get rid of two redundant ConvertToVectorRep calls
also removed a few redundant parentheses, and changed a few occurrences of Number() to Length()
The old code created five matrices: the transpose, its nullspace (which internally makes a copy of its input), a copy of the nullspace, and finally an immutable copy of the triangulized nullspace. The new code creates just three: first a mutable transpose, and then we invoke TriangulizedNullspaceMatDestructive on that, which can work in place, and returns the triangulized nullspace, of which we make an immutable copy.
While in the past MTX.Homomorphisms returned mutable matrices, this is not so anymore. In addition, we avoid one unnecessary matrix copy by replacing a TriangulizedMat call by TriangulizeMat.
Apparently this was the original implementation, and Alexander Hulpke added the new version in February 2004.
fingolfin
added a commit
that referenced
this pull request
Jan 6, 2017
Various tweaks and cleanups to the smash meataxe code, with some minor speed improvements.
olexandr-konovalov
added
the
release notes: added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Various tweaks and cleanups to the smash meataxe code, with some minor speed improvements (this time around, I didn't do extensive benchmarking, though).
While I tried to be very careful, I am not 100% certain that I made no mistakes. My plan is to wait for CodeCov to see which parts of the code that I touched is not covered by any tests, and then contribute some tests.