-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
fan isomorphism check #13189
Comments
This comment has been minimized.
This comment has been minimized.
Attachment: trac_13189_Hirzebruch_Jung_continued_fraction.patch.gz Updated patch |
Updated patch |
This comment has been minimized.
This comment has been minimized.
comment:2
Attachment: trac_13189_virtual_rays.patch.gz |
Updated patch |
comment:3
Attachment: trac_13189_fan_isomorphism.patch.gz Computing the graph automorphism group goes through GAP, which is slow. The updated patch uses a special version of the isomorphism check for 2-d fans which avoids this. |
comment:4
I thought Robert Miller wrote a very fast graph automorphism group code for Sage - am I confusing it with something else? |
comment:5
There is very nice code to compute one particular graph isomorphism, but I want to iterate over all graph isomorphisms. I'm doing this by combining the chose iso with the automorphisms of one of the graphs. But enumerating the automorphism group is using GAP, presumably you can gain more through group theory than what you can gain by making the graph theory fast. |
Reviewer: Andrey Novoseltsev |
Dependencies: #12544 |
comment:7
Glanced through, spotted a few typos that I'll fix in the reviewer patch. What do you mean by the following change of output description??
Do you mind if I also switch computation of the virtual rays to the fan constructor and allow user to specify them? It is convenient e.g. when considering an affine toric variety corresponding to a face of another cone, or a subfanfan with similar structure. Then coordinates on the smaller variety can match the bigger ones. |
comment:8
Got one error testing
The new module also has to be included into documentation, I think. Is there actually a particular reason why it is not just in |
comment:9
Replying to @novoselt:
I'm trying to say that it returns whether the two fans are equivalent up to a
If you want to implement that, go for it. |
comment:10
Replying to @novoselt:
The |
This comment has been minimized.
This comment has been minimized.
comment:11
Tests pass now. The first patch is OK modulo changes, going through others... |
Attachment: trac_13189_reviewer.patch.gz |
comment:12
OK, positive review to Volker's patches modulo reviewer's one, which needs review now. Changes:
Also, am I right that with automatically chosen virtual rays the choice cannot affect the isomorphism of cones? |
Changed keywords from none to toric |
comment:13
For the record: I have removed trailing whitespaces on new lines in the reviewer patch, so I don't think that patchbot should complain. As far as I know, ticket numbers are automatically added, so it should not complain either. And all tests pass, patchbot errors are not related. |
Added commit message |
comment:14
Attachment: trac_13189_cone_isomorphism.patch.gz I forgot the commit message in trac_13189_cone_isomorphism.patch, no actual code changes. The reviewer patch looks good to me. The virtual ray choice doesn't change whether or not there is a isomorphism of two cones / two fans (barring any bugs), but the matrix entries of the lattice map of course differ. |
Merged: sage-5.3.beta2 |
This patch implements testing for isomorphism (equivalence up to
GL(n,ZZ)
rotation) of fansThis is implemented by first computing the isomorphisms of auxiliary labelled graphs, and then trying to lift those to actual fan morphisms.
Apply:
Depends on #12544
CC: @novoselt
Component: algebraic geometry
Keywords: toric
Author: Volker Braun
Reviewer: Andrey Novoseltsev
Merged: sage-5.3.beta2
Issue created by migration from https://trac.sagemath.org/ticket/13189
The text was updated successfully, but these errors were encountered: