Closed 156bf7a1-cdbf-4d2b-a578-797e3af0730c closed 9 years ago
Branch pushed to git repo; I updated commit sha1. New commits:
e5560be | Method seems to work, docstring to go |
Commit: e5560be
Branch pushed to git repo; I updated commit sha1. New commits:
ec5421c | Fixed a bug |
Branch pushed to git repo; I updated commit sha1. New commits:
109e5bb | Separated out lift_cross_ratios() as a helper function |
Just some more testing to do, perhaps some more examples in the docstring, and probably the new helper function should really be moved to utilities.py
.
Hi Rudi,
I think you really need to fix #15296 before building code that makes it more likely for people to use cross-ratio functionality. In particular if you create the near-regular partial field as a field of fractions of a polynomial ring, things go quite horribly wrong.
Hi Stefan,
To my mind, this patch makes it less likely that people will need to compute over fields of fractions. They can now use some finite field to compute with for most of the time, and lift the representation afterwards. The lifting merely gives you different perspectives on the same algebraic object (the way I set up this method it can also be used to e.g. directly get different representations over GF(5) out of one finite-field matrix which encodes a hydra-k representation.)
But if you want to see #15296 fixed urgently, I promise you a fast review when you are done :). But seriously, I have a method in mind that will not compromise the efficiency for fields that do not need equivalence testing, but that needs certain trickery to work. I know how to do it in c++, not sure about python/cython. Perhaps you could assist me with that. I'm taking that discussion to #15296.
I will finish this patch first. Hope you can do the review!
Rudi
Branch pushed to git repo; I updated commit sha1. New commits:
3f41454 | moves helper to utilities.py, added a few generators for lift maps |
Branch pushed to git repo; I updated commit sha1. New commits:
870d476 | Combined functions to create lift maps in to one |
Branch pushed to git repo; I updated commit sha1. New commits:
c9090cf | Merge branch 'develop' into t/18624/implement_the_lift_theorem_for_linear_matroids |
Good to go, I think.
Branch pushed to git repo; I updated commit sha1. New commits:
edd18ed | finished auxiliary function, added documentation |
The patchbot perhaps needs a nudge.
Branch pushed to git repo; I updated commit sha1. New commits:
995e9b6 | doctest fixes |
The patchbot was not pleased, hope this will solve the problem.
One major thing: lift_map
needs doctests.
A couple of minor things (all but the first two you can ignore):
[refname]_
.http://arxiv.org/abs/1203.0910
, you can (should) use arxiv:`1203.0910`
. You can also make `(-i, i)`
for math formatting.INPUT:
of lift_map
should be --
instead of the comma ,
..
in the OUTPUT:
section of lift_map
since it is not a sentence.lift_cross_matrix
).Stefan, are you going to be doing the rest of the review?
Branch pushed to git repo; I updated commit sha1. New commits:
00bc825 | removed whitespace, repaired reference, added doctest in lift_map() |
Replying to @tscrim:
One major thing:
lift_map
needs doctests.A couple of minor things (all but the first two you can ignore):
- Please remove all trailing whitespace.
- Citations should be make by
[refname]_
.- I know it's not part of this ticket, but instead of
http://arxiv.org/abs/1203.0910
, you can (should) usearxiv:`1203.0910`
. You can also make`(-i, i)`
for math formatting.- You should try to make all lines under 80 characters; in particular, the moved citation.
- In the
INPUT:
oflift_map
should be--
instead of the comma,
.- I'd remove the period
.
in theOUTPUT:
section oflift_map
since it is not a sentence.- Don't be afraid of using (inline) math formatting/latex (mainly I'm thinking of
lift_cross_matrix
).
Thanks. I think my last commit covers each of these issues except for the last one.
Stefan, are you going to be doing the rest of the review?
Michael Welsh also knows this theorem well, so perhaps he is interested in reviewing this.
The code looks good (no running capability at the moment though). If Stefan doesn't beat me to it, I'll give it a proper go tomorrow.
One thing: Why are the imports interspersed throughout the code, instead of collected altogether at the top. I don't know what the convention is though.
Replying to @sagetrac-yomcat:
The code looks good (no running capability at the moment though). If Stefan doesn't beat me to it, I'll give it a proper go tomorrow.
Thanks!
One thing: Why are the imports interspersed throughout the code, instead of collected altogether at the top. I don't know what the convention is though.
Neither do I. Actually, I also don't know whether it makes any difference for the compiled code either.
Replying to @sagetrac-Rudi:
Replying to @sagetrac-yomcat:
The code looks good (no running capability at the moment though). If Stefan doesn't beat me to it, I'll give it a proper go tomorrow.
Thanks!
Two things I missed on my quick lookthrough; both are in lift_map
. Your missing a blankline after EXAMPLES::
and bullet point lists need to be aligned as:
- this is a long item
which is aligned like this
- here is the second item
One thing I messed up on is :arxiv:`1203.0910`
(I missed the leading colon). Also don't include the triple { } braces (they are just here so that it displays properly).
One thing: Why are the imports interspersed throughout the code, instead of collected altogether at the top. I don't know what the convention is though.
Neither do I. Actually, I also don't know whether it makes any difference for the compiled code either.
It takes more time to have an import inside of a method because python has to resolve that import (or seeing it already has been imported) each time the method is called. However one sometimes needs to do this to take care of circular imports: file A imports something from file B, but file B also needs to import something from file A (or potentially larger cycles).
Replying to @tscrim:
Two things I missed on my quick lookthrough; both are in
lift_map
. Your missing a blankline afterEXAMPLES::
and bullet point lists need to be aligned as:- this is a long item which is aligned like this - here is the second item
I was just touching up the docstring a bit to ue more latex, so this is solved now.
One thing I messed up on is
:arxiv:`1203.0910`
(I missed the leading colon). Also don't include the triple { } braces (they are just here so that it displays properly).
Done.
One thing: Why are the imports interspersed throughout the code, instead of collected altogether at the top. I don't know what the convention is though.
Neither do I. Actually, I also don't know whether it makes any difference for the compiled code either.
It takes more time to have an import inside of a method because python has to resolve that import (or seeing it already has been imported) each time the method is called. However one sometimes needs to do this to take care of circular imports: file A imports something from file B, but file B also needs to import something from file A (or potentially larger cycles).
Thanks for clarifying that. Moved the imports to the top.
You still need to fix the bullet points in lift_map
(before and now it is correct in lift_cross_ratios
. You can use \GF{19}
instead of GF(19)
for better latex formatting. You also need to remove the { } braces from the :arxiv: command.
Branch pushed to git repo; I updated commit sha1. New commits:
176b514 | more docstring adjustments |
Branch pushed to git repo; I updated commit sha1. New commits:
a91dc64 | begone {{{ }}} |
Thanks for your patience :)
Looks good to me.
Upon running a git trac review
, there's lots of grumpy red whitespace showing up. I think that needs to be cleaned up.
Branch pushed to git repo; I updated commit sha1. New commits:
eb67a92 | removed trailing whitespace, restated condition of lift theorem |
Replying to @sagetrac-yomcat:
Looks good to me.
Upon running a
git trac review
, there's lots of grumpy red whitespace showing up. I think that needs to be cleaned up.
The trailing whitespace is now gone, and I gave the statement of the lift theorem a bit more thought.
Branch pushed to git repo; I updated commit sha1. New commits:
3ec5a71 | in should be \in |
A few more typos/formatting changes. I think this is the correct way to format them (ignore the spaces at the beginning of lines, they're too annoying to make right):
--- Version 1
+++ Version 2
@@ -1,15 +1,15 @@
For a lift map `f` and a matrix `A` these conditions are as follows. First of all
`f: S \rightarrow T`, where `S` is a set of invertible elements of the source ring and
`T` is a set of invertible elements of the target ring. The matrix `A` has entries
- from the source ring, and each crossratio of `A` is contained in `S`. Moreover:
+ from the source ring, and each cross ratio of `A` is contained in `S`. Moreover:
- - `1 \in S`, `1\in T`;
+ - `1 \in S`, `1 \in T`;
- for all `x \in S`: `f(x) = 1` if and only if `x = 1`;
- - for all `x, y\in S`: if `x+y = 0` then `f(x)+f(y)=0`;
- - for all `x, y\in S`: if `x+y = 1` then `f(x)+f(y)=1`;
- - for all `x, y, z\in S`: if `xy = z` then `f(x)f(y)=f(z)`.
+ - for all `x, y \in S`: if `x + y = 0` then `f(x) + f(y) = 0`;
+ - for all `x, y \in S`: if `x + y = 1` then `f(x) + f(y) = 1`;
+ - for all `x, y, z \in S`: if `xy = z` then `f(x)f(y) = f(z)`.
- Any ring homorphism `h: P \rightarrow R` induces a lift map from the set of units `S` of
+ Any ring homomorphism `h: P \rightarrow R` induces a lift map from the set of units `S` of
`P` to the set of units `T` of `R`. There exist lift maps which do not arise in
this manner. Several such maps can be created by the function
:meth:`lift_map() <sage.matroids.utilities.lift_map>`.
Branch pushed to git repo; I updated commit sha1. New commits:
d46d8e3 | Docstring formatting |
The `lift theorem' gives conditions under which a matroid representation over a partial field can be transformed to a representation over another partial field, using a lift function to map the cross ratios.
Regardless whether these conditions are met, there is a polynomial-time procedure to construct a candidate representation over the target partial field, given a lift function. If the conditions of the theorem are met, then this candidate representation is the right one.
Write a method for LinearMatroid, which given a lift function, generates such a candidate representation.
CC: @sagetrac-Stefan @sagetrac-yomcat
Component: matroid theory
Author: Rudi Pendavingh
Branch/Commit:
7f020ea
Reviewer: Michael Welsh
Issue created by migration from https://trac.sagemath.org/ticket/18624