Opened 8 years ago

Closed 7 years ago

#12544 closed enhancement (fixed)

Switch cones to `PointCollection`

Reported by: novoselt Owned by: mhampton
Priority: major Milestone: sage-5.2
Component: geometry Keywords: toric, sd40.5
Cc: vbraun Merged in: sage-5.2.beta1
Authors: Andrey Novoseltsev Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12965 Stopgaps:

Description (last modified by jdemeyer)

The class PointCollection at #11400 is intended for uniform handling of ray/line generators and their subsets for cones, fans, and, eventually, lattice polytopes. This ticket is about making the switch for cones.

Apply: trac_12544_switch_cones_to_PointCollection_folded.patch

Attachments (5)

trac_12544_switch_cones_to_PointCollection.patch (12.1 KB) - added by novoselt 8 years ago.
trac_12544_fix_doctests.patch (25.3 KB) - added by novoselt 8 years ago.
trac_12544_deprecate_old_ray_methods.patch (69.4 KB) - added by novoselt 8 years ago.
trac_12544_flip_point_collection_matrix.patch (12.3 KB) - added by novoselt 8 years ago.
trac_12544_switch_cones_to_PointCollection_folded.patch (118.0 KB) - added by novoselt 7 years ago.

Download all attachments as: .zip

Change History (47)

comment:1 Changed 8 years ago by novoselt

So far the patch is "proof of concept" and with the alternative solution on #12541 it passes all the tests for cones and fans (except for like 60 fails due to changed output formatting). This is exactly what was intended: most things should work as if cone.rays() was still a tuple, but now we can tweak representation and latexing, add extra methods, and suffer no performance hit as it would be with Sequence.

comment:2 Changed 8 years ago by novoselt

  • Dependencies set to #12541

comment:3 Changed 8 years ago by novoselt

  • Dependencies changed from #12541 to #11599, #12541
  • Description modified (diff)
  • Status changed from new to needs_review

I've tried to break changes into several patches, although they got bigger and bigger. However, despite of the total size, these patches should be easy to review as they don't do anything deep - a few conversion methods, a few deprecations, and a lot of doctest changes due to different output formatting.

Personally, I am quite pleased with the results - while some calls got a few more characters, one of the most important methods for cones and fans now produces convenient output with a detailed but not repeated description of the ambient lattice. This worked especially well for quotients and sublattices, whose description was not obvious from ray output before. Hopefully, that's not just my point of view and the changes are objectively for the best ;-)

I have #11599 in the beginning of my queue now, so this ticket is based on top of it.

comment:4 Changed 8 years ago by novoselt

  • Dependencies changed from #11599, #12541 to #11599, #11634, #12541

Rebased to apply on top of the positively reviewed #11634

comment:5 Changed 8 years ago by novoselt

... and removed added trailing whitespaces.

comment:6 Changed 8 years ago by davidloeffler

  • Status changed from needs_review to needs_work
applying trac_12544_deprecate_old_ray_methods.patch
patching file sage/geometry/cone.py
Hunk #42 FAILED at 3787
1 out of 44 hunks FAILED -- saving rejects to file sage/geometry/cone.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_12544_deprecate_old_ray_methods.patch

(My guess is that you removed a trailing whitespace from a line in an earlier patch that was used as a context line by this later one.)

comment:7 Changed 8 years ago by novoselt

  • Status changed from needs_work to needs_review

Grrr... That's precisely what has happened. New versions are made using hg export and seem to apply fine...

comment:8 Changed 8 years ago by novoselt

  • Dependencies changed from #11599, #11634, #12541 to #11599, #11634, #12541, #12361

Rebased on top of #12361, which is almost done.

comment:9 Changed 8 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to patch does not apply

According to the patchbot this is failing to apply to 5.0.beta10 -- any idea what that's about?

comment:10 Changed 8 years ago by novoselt

I don't know - maybe I have saved some changes in the previous patches as well. I've reexported versions that apply to beta10 for me without any problems.

comment:11 Changed 8 years ago by davidloeffler

  • Status changed from needs_work to needs_review
  • Work issues patch does not apply deleted

Changed 8 years ago by novoselt

Changed 8 years ago by novoselt

comment:12 Changed 8 years ago by novoselt

OK - I think it was a glitch on my part with synchronisation of different queues and when I've done the rebasing on top of cohomology patches I used versions with wrong whitespacing. Hopefully the new version applies smoothly and passes whitespace test...

comment:13 Changed 8 years ago by novoselt

Question to the patchbot - how so?..

Looking at #11634
#11634 already applied (5.0.beta10 <= 5.0.beta3)

comment:14 Changed 8 years ago by davidloeffler

This is a patchbot bug: its code for sorting version numbers splits on "." and then does a string sort, so it comes unstuck on betaN for multi-digit N. There are three or four copies of the patchbot running, with different Sage versions and various combinations of plugins and other hacks. I've hacked mine (that's "fermat") to fix this sorting issue, but the patchbot running beta3 on sagenb.kaist.ac.kr is still unpatched. See #12486.

Changed 8 years ago by novoselt

comment:15 Changed 8 years ago by novoselt

After some work on switching lattice polytopes also to point collections, I've finally realized that it is not a good idea to have cone.rays().matrix() return matrix with rays as columns, one of the main reasons is that matrix(list(cone.rays())) gives the opposite representation with rays as rows. The main motivation for the original format was convenience to look at rays/points, but with a specialized container we can easily transpose the matrix in repr/latex, so I have done precisely that: now matrix(cone.rays()) == cone.rays().matrix() and cone.rays()[i] == cone.rays().matrix()[i].

comment:16 Changed 8 years ago by novoselt

  • Description modified (diff)

As the last patch of the sequence changes many places changed by previous ones, I've folded everything into a single one. All tests still pass!

Apply trac_12544_switch_cones_to_PointCollection_folded.patch

comment:17 Changed 7 years ago by vbraun

I'm fine with switching to PointCollection, but I think the matrix notation for the cone rays is a step backwards:

  • It is not copy-pastable any more
  • Unreadable as soon as you exceed the terminal width
  • Confusing because cone.rays().matrix() is transposed relative to cone._repr_().

I think it would be better to keep the string representation for cones the same.

comment:18 Changed 7 years ago by novoselt

Hmmm... I actually find the current notation not very readable beyond 2-3 rays, certainly not when the output does not fit one line and for like 20 rays in 3 or 4 dimensions I definitely prefer a horizontal matrix in the notebook. For small ones it seems then to be more convenient to be the same and if we are talking about hundreds of rays there is probably no convenient notation, although somehow aligned structure still seems to be preferable.

Anyway, I think this is in a big part personal preference and one of the points of point collections is ability of customisation. How about I'll add some method that allows one to switch globally between ray-by-ray and (optionally transposed) matrix representation?

The default can be ray-by-ray as it is now or perhaps organized in such a way that each ray is on its own line and components are aligned in columns, but still with all the parentheses and commas to make it copy-pastable. Or it can be what I've done now, but as soon as you exceed 80 (or 60) characters it switches to ray list automatically. Those who don't like the default can adjust it in the beginning of their worksheets ;-)

I'll also would like to keep in any case "in 3-d lattice N" or whatever on the last line: I think this looks fine with "regular lattices" and is very useful for more complicated ones such as sublattices, quotients, class groups. So with proposed alignment it could look like

sage: cone.rays()
N(-3, 345,     1),
N( 1,   0, -8897)
in 3-d lattice N

What do you think?

comment:19 Changed 7 years ago by vbraun

I'm against global switches, thats just a way of saying "we haven't thought this through" ;-) Also, if you want a matrix then you can just write rays().matrix().

Your proposed column-aligned output would be fine with me.

comment:20 follow-up: Changed 7 years ago by novoselt

  • Status changed from needs_review to needs_work
  • Work issues set to output formatting

The matrix which I want to see is rays().matrix().transpose() and typing it every time instead of just rays() is annoying. Plus I'd like to see that lattice indication, so I need to output it with lattice() and since it is a double output I need to explicitly say print or show...

We have thought it through, but have different opinions ;-) Which (in my opinion ;-)) indicates that other people also can prefer one or another. Just as some may prefer terminal, others text notebook, and others typeset notebook. We also have switches for, say, automatic symbolic variables, and some LaTeX macros are created specifically to make output customisable.

Note that in typeset notebook there is no issue of copy-pasting output (nothing is really copy-pastable) and no issue of going over the border - each cell has a scroll bar so matrices with a hundred columns are viewable just fine. This leaves only one of your three objections applicable and I agree that it is a bit confusing, but, again in my opinion, the transposed matrix is more convenient to look at. For example, it allows you to see (parts of) several horizontally-long matrices at the same time. For vertically-long ones this is impossible and often you will not see the command which has created the output.

comment:21 in reply to: ↑ 20 Changed 7 years ago by vbraun

Seriously, everybody is going to be confused by matrix vs. transpose, so that is never a sane setting. I was confused by it when I read your patch. Also, nobody is going to look at high-dimensional coordinate vectors all day so I don't see how it puts any extra burden on you. In your own work you can always override _repr_() if you have to see a rays-as-columns matrix.

comment:22 follow-up: Changed 7 years ago by novoselt

After sleeping on it and thinking some more:

Overriding _repr_ is not acceptable, as it makes computations not reproducible by others (unless they also do the same overriding, but understanding it requires some knowledge of Sage internal conventions).

Looking at matrices with rays/points/vectors as columns is not just my invention, e.g. PALP, which is familiar to a number of people, does it as well as long as the matrix fits the terminal. Then it switches to row vectors (the switch, I think, is quite confusing). It is also quite common practice in linear algebra as I was reminded recently having to cover some classes. And that's why Rob Beezer added column_matrix constructor.

So I think that:

  • The representation should not change depending on dimension or number of rays.
  • To make it fit on the terminal, it is better to use one line per ray.
  • I still think that it is the best for typeset notebook version to have rays as columns, so I really want to have it as an option.
  • I don't see what is the problem with having a function like sage.geometry.point_collection.default_representation which allows you switch between lists and matrices or anything else, it just makes Sage more flexible and provides a standard way of adjusting it rather than hacking its guts.
  • It is indeed confusing when rays.matrix() returns the transpose of the matrix in representation.

To address all of this, I propose:

  • Default text output as was shown above, in particular, that's what will be used in all doctests.
  • Default LaTeX output similar to it, packing it into array for alignment.
  • Add a method rays.column_matrix() which returns the matrix with rays as columns. This would make it consistent with matrix(rays) and column_matrix(rays).
  • Add a function for changing the output to other forms, most of all "rays as columns" which will refer to the method column_matrix to describe it: all confusion should be gone, I'll be happy, and the default output will be as you prefer, so to me it sounds like a reasonable compromise.

It could be awesome to draw vertical lines between columns to indicate the direction of rays, but jsMath swallows them, so what's the point...

comment:23 in reply to: ↑ 22 Changed 7 years ago by vbraun

Replying to novoselt:

PALP, which is familiar to a number of people, does it as well as long as the matrix fits the terminal. Then it switches to row vectors (the switch, I think, is quite confusing).

Thats why PALP suddenly returns transposed matrices?? Is that anywhere documented? I've observed this behavior but never figured out why PALP does that. That is really a terrible design choice for a program so technical that most people will want to parse its output.

  • I don't see what is the problem with having a function like sage.geometry.point_collection.default_representation which allows you switch between lists and matrices or anything else

The way I see it, the only difference between

sage.geometry.point_collection.default_representation = my_print_func

and

sage.geometry.cone.ConvexRationalPolyhedralCone._repr_ = my_print_func

is that I'll never discover the former, while the latter is the familiar monkey-patching that you can do with any Python module. If you really want to call it default_representation then I'm not totally opposed, but nobody is going to use it because nobody is going to find out that its there.

comment:24 Changed 7 years ago by novoselt

  • Work issues changed from output formatting to output formatting, base rings

This patch breaks the following code:

P3 = toric_varieties.P(3)
Sigma3 = P3.fan()
point_rays = [sum(sigma.rays()) for sigma in Sigma3]
BlP3 = P3.resolve(new_rays=point_rays)
K = BlP3.Kaehler_cone()
K._split_ambient_lattice()

The reason is that the ray matrix is constructed over rational field (because K lives in the rational divisor class group) and Smith form does not do what was intended. I am not yet sure what has to be tweaked here. It seems that creating a cone explicitly in some rational vector space V rather than some sublattice of it is completely reasonable. But it is still possible to choose ray generators in the sublattice, I guess in span(ZZ, V.gens())?..

comment:25 Changed 7 years ago by novoselt

OK, how about this variant:

  • class method PointCollection.output_format that affects both text and LaTeX representation and is visible to those who will look at TAB-completion;
  • default text representation as described above, with line breaks and coordinate alignment;
  • default LaTeX representation as a tuple (i.e. as it was but with lattice subscript) - aligning coordinates which are wrapped in \left( and \right) it a bit hard, those who want alignment are welcome to switch to matrices.

The new patch does all this and fixes all doctests in point_collection module so they show how it works now. If you are OK with this look and approach, I'll fix doctest formatting in all other files.

comment:26 Changed 7 years ago by vbraun

  • Keywords sd40.5 added

comment:27 Changed 7 years ago by novoselt

  • Dependencies changed from #11599, #11634, #12541, #12361 to #11599, #11634, #12541, #12361, #12965

comment:28 Changed 7 years ago by novoselt

Rebased to the new position of toric files.

Volker, let me know if new formatting is OK and I finish doctest fixing.

comment:29 Changed 7 years ago by vbraun

Yes looks good to me!

comment:30 Changed 7 years ago by novoselt

  • Status changed from needs_work to needs_review
  • Work issues output formatting, base rings deleted

OK, ready to go!

comment:31 Changed 7 years ago by novoselt

Patchbot, apply only trac_12544_switch_cones_to_PointCollection_folded.patch

comment:32 Changed 7 years ago by novoselt

Apply trac_12544_switch_cones_to_PointCollection_folded.patch

comment:33 Changed 7 years ago by vbraun

Apply trac_12544_switch_cones_to_PointCollection_folded.patch

comment:34 Changed 7 years ago by vbraun

  • Description modified (diff)

comment:35 Changed 7 years ago by novoselt

The patchbot positively hates this ticket and does not want to understand that only last patch has to be applied...

comment:36 Changed 7 years ago by vbraun

  • Milestone changed from sage-5.1 to sage-5.2
  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

Looks good to me.

comment:37 Changed 7 years ago by jdemeyer

  • Dependencies changed from #11599, #11634, #12541, #12361, #12965 to #12965

comment:38 Changed 7 years ago by robertwb

Apply only trac_12544_switch_cones_to_PointCollection_folded.patch

comment:39 Changed 7 years ago by robertwb

apply trac_12544_switch_cones_to_PointCollection_folded.patch

comment:40 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:41 Changed 7 years ago by novoselt

So - any clue what is wrong with the patchbot here?

comment:42 Changed 7 years ago by jdemeyer

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