Opened 10 years ago

Closed 10 years ago

#11385 closed enhancement (fixed)

Orbit closure as toric variety

Reported by: vbraun Owned by: AlexGhitza
Priority: major Milestone: sage-4.7.2
Component: algebraic geometry Keywords: sd31
Cc: novoselt Merged in: sage-4.7.2.alpha3
Authors: Volker Braun, Andrey Novoseltsev Reviewers: Andrey Novoseltsev, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by novoselt)

The closure of the torus orbit associated to a cone of the fan is again a toric variety. This patch implements a method toric_variety.orbit_closure(cone) to construct this toric variety.

Apply:

  1. trac_11385_orbit_closure_as_toric_variety.patch
  2. trac_11385_reviewer.patch

Attachments (2)

trac_11385_orbit_closure_as_toric_variety.patch (4.0 KB) - added by vbraun 10 years ago.
Updated patch
trac_11385_reviewer.patch (8.4 KB) - added by novoselt 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by vbraun

  • Authors set to Volker Braun
  • Cc novoselt added
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev

Um, I don't really understand the documentation of the first (private) function. Could you please expand/rewrite it a bit?-) In particular the INPUT section is a bit confusing given that there are two parameters and one of them is already called cone.

As a possible optimization

return ToricVariety(Fan(cones))

can be replaced with

return ToricVariety(Fan(discard_faces(cones), check=False))

where discard_faces is added to the fan module by #11200.

comment:3 Changed 10 years ago by novoselt

  • Keywords sd31 added
  • Status changed from needs_review to needs_work
  • Work issues set to documentation clarification

Changed 10 years ago by vbraun

Updated patch

comment:4 Changed 10 years ago by vbraun

  • Status changed from needs_work to needs_review

I've added some documentation and a reference to [CLS].

comment:5 Changed 10 years ago by SimonPL

Note that this is my first day in sage package reviewing, so take this with a grain of salt. However, I did manage to apply another patch correctly before this one.

When I hg.apply() the patch, I get the following error :

 hg_sage.apply("/home/simon/sage/devel/sage-test/patches/trac_11385_orbit_cl>
cd "/home/simon/sage/devel/sage" && hg status
cd "/home/simon/sage/devel/sage" && hg status
cd "/home/simon/sage/devel/sage" && hg import   "/home/simon/sage/devel/sage-test/patches/trac_11385_orbit_closure_as_toric_variety.patch"
applying /home/simon/sage/devel/sage-test/patches/trac_11385_orbit_closure_as_toric_variety.patch
internal patcher failed
please report details to http://mercurial.selenic.com/bts/
or mercurial@selenic.com
/home/simon/sage/local/bin/patch: **** Only garbage was found in the patch input.
abort: patch command failed: exited with status 512

comment:6 Changed 10 years ago by vbraun

Thanks for having a look! Instead of apply, its probably better to use import_patch like this:

sage: hg_sage.import_patch('http://trac.sagemath.org/sage_trac/raw-attachment/ticket/11385/trac_11385_orbit_closure_as_toric_variety.patch')

You probably downloaded the html-formatted patch instead of the raw patch file.

If you want to get more serious about Sage development then you should read about mercurial queues, though. This is a much better way to handle multiple patches.

comment:7 Changed 10 years ago by novoselt

  • Work issues documentation clarification deleted

For the record - I have not abandoned this patch ;-) I would be happier if the default was to construct the fan of the orbit closure in the honest quotient lattice. I suspect Volker has not done it this way because it just does not work. Well, I am trying to make it work, since it will be yet another step in the direction of the full support of quotients and sublattices. I think they should be freely usable everywhere where lattices can be used, because, well, they are lattices as well ;-)

comment:8 Changed 10 years ago by vbraun

  • Milestone changed from sage-4.7.1 to sage-4.7.2

Making it work properly with quotient lattices is a noble goal, of course.

Another question is where to put the method to restrict line bundles from the ambient to the orbit closure. Because of the quotient lattice it is not literally pull-back by a toric morphism. I tend to think that ToricOrbitClosure should derive from ToricVariety and add a restrict_divisor() method.

Though as far as this ticket is concerned, I think we all agree that there should be a .orbit_closure(cone) method for toric varieties that returns a toric variety. So maybe we should merge this ticket before improving the presentation of the resulting toric variety.

Changed 10 years ago by novoselt

comment:9 Changed 10 years ago by novoselt

  • Authors changed from Volker Braun to Volker Braun, Andrey Novoseltsev
  • Description modified (diff)
  • Reviewers changed from Andrey Novoseltsev to Andrey Novoseltsev, Volker Braun

Ideally the toric variety of the orbit closure should come with the canonical embedding, even though it does not come from a fan morphism. So I think it would be better to have the restriction directly in the toric variety class.

The attached patch contains bits of code that move us closer to supporting quotient lattices, so I propose to add them - I wrote them while making the commented code work. It still does not work for cones, so I agree to leave the rest of necessary changes for the future. I also changed the docstrings quite a bit, hopefully for the best ;-)

If you are OK with the new stuff, please switch to positive review.

comment:10 Changed 10 years ago by vbraun

  • Status changed from needs_review to positive_review

looks good!

comment:11 Changed 10 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.