Opened 4 years ago
Closed 2 years ago
#26623 closed enhancement (fixed)
Constructions for common polyhedral cones
Reported by:  mjo  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  geometry  Keywords:  
Cc:  novoselt  Merged in:  
Authors:  Michael Orlitzky  Reviewers:  Jonathan Kliem 
Report Upstream:  N/A  Work issues:  
Branch:  b2aab91 (Commits, GitHub, GitLab)  Commit:  b2aab916b145144f10f325161c551301ac34c351 
Dependencies:  Stopgaps: 
Description
When working with graphs, we have a nice tabcompleted list of common graphs that we can construct; for example,
sage: graphs.BuckyBall() Bucky Ball: Graph on 60 vertices
More and more, I find myself wishing we had the same thing for convex cones, particularly in test cases. I have constructions for all of the following and use them regularly:
 The nonnegative orthant.
 The full ambient space.
 The schur cone that induces the majorization ordering.
 The permutationinvariant "rearrangement" cone.
 The trivial cone, since
trivial_cone(n)
looks a lot nicer thanCone([], ToricLattice(n))
.
et cetera; I'm sure I can think of more. Each of these makes sense in any dimension n
.
I'm wondering if you think this would be a useful feature to make available as e.g.
sage: cones.nonnegative_orthant(5) 5d cone in 5d lattice N
They should all probably take a "lattice" argument too now that I think about it.
Change History (51)
comment:1 followup: ↓ 2 Changed 4 years ago by
comment:2 in reply to: ↑ 1 Changed 4 years ago by
Replying to novoselt:
They certainly should take a lattice argument! From a toric perspective it is probably more natural to have these as methods of lattices, i.e. something like
ToricLattice(n).trivial_cone()although of course this makes it impossible to use
ZZ^n
.
I think from a design standpoint, that's where the method belongs, but there are two things that bug me about it.
First: in many cases, the user won't care about the lattice and will want "the default." This is basically what happens when you do ToricLattice(n).nonnegative_orthant()
, but it feels a little weird to specify a particular lattice when you don't care about it. By analogy, the Cone()
function should also be a method on a ToricLattice
object. A lot of the time you just don't care though, so it's nice to be able to get a sensible default without having to construct a generic lattice of the correct size first. On the other hand, I like that this eliminates the n
and lattice
parameters from the conecreating functions. And it would ensure that (for example) if you only create one lattice object, then you can intersect any two cones created by methods on it. I'll have to think harder about this but it may be the way to go.
Second: tab completion on graphs.<tab>
is so nice. Having a list of twenty or so cones interspersed in the tabcompletion list for lattices would be bad for both people who want to see lattice methods and people who want to see predefined cones.
(Might we have something like lattice.cones.<tab>
where lattice
is some ToricLattice
?)
And perhaps even more natural is to have not cones, but associated toric varieties which are already available:
sage: toric_varieties.A(5).fan().generating_cone(0).rays() ...
My main objection to this is that I'm not smart enough to implement it or remember which cone is associated with which toric variety =)
Having the code above be the implementation for nonnegative_orthant(5)
would be fine with me though.
comment:3 Changed 4 years ago by
"A lot of the time you just don't care though"  I always care, we are just using cones for different things ;) But you are right  just because cones are associated to lattices does not mean that they have to be methods of lattices. So I think cones.etc
is the right interface, but there has to be an option to specify the lattice and the default should be the same as for Cone(...)
.
comment:4 Changed 4 years ago by
I have another question. There are a few vector spaces associated with each cone, that can be thought of as cones themselves:
 The span of the cone K.
 Its orthogonal complement, (span(K))perp.
 Its lineality space (the largest subspace it contains).
The span of the cone and its lineality space are already available as,
sage: K = Cone([(1,1),(0,1)]) sage: K.span() Sublattice <N(1, 0), N(0, 1)> sage: span(K) Free module of degree 2 and rank 2 over Integer Ring Echelon basis matrix: [1 0] [0 1] sage: K.linear_subspace() Vector space of degree 2 and dimension 0 over Rational Field Basis matrix: []
But none of those return cones directly. Having a cone is handy when you want to compute e.g. the intersection of one of these things with some other cone.
If we were to add similar methods that return cones, how would you prefer to do it? We could either...
 Add methods to the cones themselves, for things like
K.span_cone()
,K.lineality_space()
,K.orthogonal_complement()
 Add them as part of the new
cones.<whatever>
interface, with something likecones.span_of(K)
.
The first one seems more correct to me, but risks polluting the namespace with a bunch of methods that all look the same but return different things.
The second one does make it obvious that you're going to get a cone back, but only if people know to look there for what should in principle be a method on the cone itself.
comment:5 Changed 4 years ago by
Option 3:
For the span and the orthogonal complement, we could sensibly add a cone()
method somewhere in the module hierarchy, like
sage: K.orthogonal_sublattice().cone()
which would be a wrapper around
sage: Cone( c*g for g in K.orthogonal_sublattice().gens() for c in [1, 1] )
The problem with that is, when starting from a cone and passing through a vector space, the lattice information is lost. If I start with a cone in a lattice M
, take its linear_subspace()
, and then turn it into cone...
sage: K = Cone([(1,0,0),(1,0,0),(0,1,0)], ToricLattice(3,'M')) sage: Cone(K.linear_subspace().gens()) 1d cone in 3d lattice N
it should live in the same lattice as the original cone. I don't see how to fix that immediately.
comment:6 Changed 4 years ago by
Just a reminder  orthogonality in the toric setting is handled via dual lattices:
sage: c = Cone([(1,2,3), (4,5,6)]) sage: c 2d cone in 3d lattice N sage: c.orthogonal_sublattice() Sublattice <M(1, 2, 1)> sage: c.span() Sublattice <N(1, 2, 3), N(0, 3, 6)>
I can't imagine use cases where I would like to treat all these spaces as cones. Can you please explain it in more details, i.e. what is wrong with treating them as, well, spaces? If you really do want to have cones associated to spaces, then you can easily attach a method to toric lattices and sublattices, or, alternatively or in addition, you can modify the Cone
constructor to handle lattices and spaces. I just tried Cone(c.span())
for the above example and probably hit some infinite recursion that is in addition very slow  after a few minutes it is still doing something.
But I am still unclear why would one want to turn a space into a cone, what is there to gain from such a representation?
comment:7 Changed 3 years ago by
Oof, it looks like I'm missing trac emails again.
It would just be nice to have the cone methods available on subspaces. For example, Corollary 1 in these notes,
http://michael.orlitzky.com/notes/on_zoperators_and_viability_theorems.pdf
is very easy to test as the intersection of a closed convex (dual) cone and a subspace. If span(x) is a cone, and if its orthogonal complement is a cone, then I can use the cone intersection method that we already have to test it.
Another example is the "maximal angle between two convex cones." This is a newish concept (from 2016), and I've implemented it as a method on cones. However, the "principal angle between subspaces" is a classical idea going back to the 1950s, and they turn out to be equivalent for cones that are subspaces. If subspaces can be treated as cones, then I can use the same method that I have on cones to compute principal angles between subspaces. And so on.
The reason Cone(c.span())
"hangs" is because it carefully enumerates every element of the vector space.
I think there's plenty of work to do just to get the trivial cone, nonnegative orthant, and a few others implemented and documented. I'll focus on that and worry about the other questions later.
comment:8 Changed 3 years ago by
 Branch set to u/mjo/ticket/26623
 Commit set to eb072cefecfe1382929a13ee2f40c6525093b363
 Status changed from new to needs_review
Here's my first attempt at this. I'm open to other suggestions if you don't like the module or class names.
New commits:
6645911  Trac #26623: add global entries for pending sage.geometry.cones citations.

84f5635  Trac #26623: add cones.<whatever> shortcuts for common cones.

dd759dc  Trac #26623: add the sage.geometry.cones module to the documentation index.

eb072ce  Trac #26623: update doctests in cone.py to use new "cones" methods.

comment:9 followup: ↓ 10 Changed 3 years ago by
I am fine with the class names but the objects would better be like graphs (and many other constructors): that is in caml case like Trivial
, NonnegativeOrthant
, etc
comment:10 in reply to: ↑ 9 Changed 3 years ago by
Replying to vdelecroix:
I am fine with the class names but the objects would better be like graphs (and many other constructors): that is in caml case like
Trivial
,NonnegativeOrthant
, etc
Doesn't that violate PEP8? They're not really constructors, even though they do mimic constructors (in the sense that they would be constructors if I didn't want the cones.
prefix). However this is python where almost every function creates a new object and returns it, so the distinction seems a little arbitrary to me.
comment:11 Changed 3 years ago by
Please have a look at: graphs.
, designs.
, polytopes.
, manifolds.
, ... All of them use caml case and most of them are not classes. I am not talking about PEP8 but consistency within sage.
comment:12 followup: ↓ 13 Changed 3 years ago by
It seems that when you do specify a lattice, you also have to specify its dimension and there is a check that you do not make a mistake. It would be more pleasant to provide only one input argument: either a number to get the cone in the default lattice of that dimension, or a lattice and get a cone in that lattice.
comment:13 in reply to: ↑ 12 Changed 3 years ago by
Replying to novoselt:
It seems that when you do specify a lattice, you also have to specify its dimension and there is a check that you do not make a mistake. It would be more pleasant to provide only one input argument: either a number to get the cone in the default lattice of that dimension, or a lattice and get a cone in that lattice.
But that's exactly how Cone()
works, and Cone()
is the function that underlies all of these convenience methods.
The additional check is not strictly required; it serves only to provide a more relevant error message. For example, if I were to delete the check and then pass a lattice of the wrong rank to Cone()
, I would get
sage: M = ToricLattice(2) sage: Cone([(1,1,0),(0,1,1)], lattice=M) ... TypeError: cannot convert (1, 1, 0) to Vector space of dimension 2 over Rational Field!
If you know the generators of (say) the Schur cone, then you might be able to figure out what that error message means. But, if you don't know what those generators are off the top of your head, then an error message complaining about converting (1,1,0)
to a rational vector space could be confusing. That said, I'm not too attached to the additional sanity check and error message, and would be happy to remove them.
It would be more pleasant to provide only one input argument: either a number to get the cone in the default lattice of that dimension
I don't think this is workable, since in applications we often want to intersect the dual of some cone (which will live in a nondefault lattice) with e.g. a nonnegative orthant.
only one input argument... a lattice, and get a cone in that lattice.
This is the best API, but as a userinterface is pretty annoying to use. The "pass a lattice, but only if you don't want the default one" approach of Cone()
is IMO a good compromise. Having to do e.g.
sage: cones.trivial(ToricLattice(3))
is even worse than
sage: Cone([], ToricLattice(3))
In the common case, the user is left wondering why he has to type ToricLattice(n)
all the time instead of just n
. The default is usually fine until you start doing complicated stuff.
comment:14 followup: ↓ 15 Changed 3 years ago by
Cone
works for arbitrary cones and it may be convenient to give input as integer vectors, especially as a user input, in which case it is handy to specify some lattice and cast all vectors to it. Just specifying a lattice does not make much sense for Cone
except for a trivial one, perhaps. But in your constructors everything is determined by dimension, there are no explicit rays to give, so what I propose is to have two options: cones.trivial(3)
and cones.trivial(my_lattice)
. I do not suggest that you eliminate the form when only the integer is given, rather do not require this integer in those cases, when the lattice was given explicitly!
comment:15 in reply to: ↑ 14 Changed 3 years ago by
Replying to novoselt:
I do not suggest that you eliminate the form when only the integer is given, rather do not require this integer in those cases, when the lattice was given explicitly!
Ah, sorry I misunderstood. This sounds like a good idea.
comment:16 Changed 3 years ago by
 Commit changed from eb072cefecfe1382929a13ee2f40c6525093b363 to 5533bd22838f7d530f37812662e3a6353d2439a8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
53f7b42  Trac #26623: add global entries for pending sage.geometry.cones citations.

cab6be8  Trac #26623: add cones.<whatever> shortcuts for common cones.

f48f779  Trac #26623: add the sage.geometry.cones module to the documentation index.

eda8548  Trac #26623: update doctests in cone.py to use new "cones" methods.

8122dc0  Trac #26623: factor out common "cones" argument processing.

5533bd2  Trac #26623: use Pascal case for "cones" method names.

comment:17 Changed 3 years ago by
Each method will now make an attempt to determine the ambient dimension from the lattice and viceversa. An error is raised if they are both left unspecified, or if they disagree. I don't think it's necessary to prohibit e.g. nonnegative_orthant(3,M)
if the rank of M
is 3
.
I also added a commit on top of the others to switch to PascalCase? names. My mother taught me that "everyone else is doing it" isn't a good excuse, so I'm not sold on the motivation, but it's there. I don't feel that strongly about it if I'm in the minority.
comment:18 Changed 3 years ago by
 Commit changed from 5533bd22838f7d530f37812662e3a6353d2439a8 to 59fd912961474829dd39cc4edbaa462158f2f4bb
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7ddd803  Trac #26623: add global entries for pending sage.geometry.cones citations.

0588289  Trac #26623: add cones.<whatever> shortcuts for common cones.

a80699c  Trac #26623: add the sage.geometry.cones module to the documentation index.

1f8aeaf  Trac #26623: update doctests in cone.py to use new "cones" methods.

d266a33  Trac #26623: factor out common "cones" argument processing.

59fd912  Trac #26623: use Pascal case for "cones" method names.

comment:19 Changed 3 years ago by
Forcepushed a rebase onto the "develop" branch to fix a conflict in src/doc/en/reference/references/index.rst
.
comment:20 followup: ↓ 22 Changed 3 years ago by
 Status changed from needs_review to needs_work
There seems to be a conflict with version 8.9.beta8.
comment:21 Changed 3 years ago by
 Commit changed from 59fd912961474829dd39cc4edbaa462158f2f4bb to f4c577e7afa2a469ffe35997adcc99298e3f7b86
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4fcb2c4  Trac #26623: add global entries for pending sage.geometry.cones citations.

d163d52  Trac #26623: add cones.<whatever> shortcuts for common cones.

58e305f  Trac #26623: add the sage.geometry.cones module to the documentation index.

65f70a4  Trac #26623: update doctests in cone.py to use new "cones" methods.

b478929  Trac #26623: factor out common "cones" argument processing.

f4c577e  Trac #26623: use Pascal case for "cones" method names.

comment:22 in reply to: ↑ 20 Changed 3 years ago by
 Status changed from needs_work to needs_review
Replying to jipilab:
There seems to be a conflict with version 8.9.beta8.
Another conflict in the global references list. Fixed now, thanks for the heads up.
comment:23 followup: ↓ 25 Changed 3 years ago by
Would it make sense to use lazy imports?
comment:24 Changed 3 years ago by
 Commit changed from f4c577e7afa2a469ffe35997adcc99298e3f7b86 to 23b7dfca8f406bd037876fd4f8e8911c39036016
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
146bf23  Trac #26623: add global entries for pending sage.geometry.cones citations.

f8f5feb  Trac #26623: add cones.<whatever> shortcuts for common cones.

0d2d9a6  Trac #26623: add the sage.geometry.cones module to the documentation index.

29b269d  Trac #26623: update doctests in cone.py to use new "cones" methods.

5665a76  Trac #26623: factor out common "cones" argument processing.

b919f90  Trac #26623: use Pascal case for "cones" method names.

23b7dfc  Trac #26623: use a lazy_import for the global "cones" object.

comment:25 in reply to: ↑ 23 Changed 3 years ago by
Replying to ghkliem:
Would it make sense to use lazy imports?
Good idea. I dislike lazy imports when they're used to misstructure a hierarchy, but in this case it prevents sage from fully loading an esoteric feature and should speed things up.
comment:26 Changed 3 years ago by
 Commit changed from 23b7dfca8f406bd037876fd4f8e8911c39036016 to 9d605a78adf96a6eff19741b4f0090cbf39821b5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2cffd93  Trac #26623: add global entries for pending sage.geometry.cones citations.

d2e22a2  Trac #26623: add cones.<whatever> shortcuts for common cones.

211477f  Trac #26623: add the sage.geometry.cones module to the documentation index.

e707e63  Trac #26623: update doctests in cone.py to use new "cones" methods.

f0542f2  Trac #26623: factor out common "cones" argument processing.

0f4d979  Trac #26623: use Pascal case for "cones" method names.

9d605a7  Trac #26623: use a lazy_import for the global "cones" object.

comment:27 Changed 3 years ago by
Rebased onto develop branch.
comment:28 followup: ↓ 30 Changed 2 years ago by
 Status changed from needs_review to needs_work
Some minor things:
 Is CamelCase really necessary (compare the
polytopes.
library...)?  The function
_preprocess_args
needs doctests.  There is a pyflake import error, could you take care of it?
comment:29 Changed 2 years ago by
 Commit changed from 9d605a78adf96a6eff19741b4f0090cbf39821b5 to 3c880885fbc910f21b76745dea1a141e205f6ce4
Branch pushed to git repo; I updated commit sha1. New commits:
3c88088  Trac #26623: remove two unused imports.

comment:30 in reply to: ↑ 28 Changed 2 years ago by
Replying to jipilab:
Some minor things:
 Is CamelCase really necessary (compare the
polytopes.
library...)?
The hardest question to answer. I started with e.g. cones.nonnegative_orthant()
, and was asked to change it for consistency (read the earlier comments). I don't have strong feelings either way, but slightly prefer the way the lowercase/underscore names look. We should have a consistent, documented policy about this. I started a thread on dev about it.
 The function
_preprocess_args
needs doctests.
This is doctested, it just doesn't look like it. That code was originally inlined into each individual method, and each individual method still has tests to make sure that it works. You can check e.g. the last three TESTS for Schur()
to see that they're really testing the code in _preprocess_args
. This actually results in some duplicated tests, but makes sure that I haven't totally forgotten to call _preprocess_args
for some cone (and is nice documentation).
 There is a pyflake import error, could you take care of it?
Fixed, thanks.
comment:31 Changed 2 years ago by
 Branch changed from u/mjo/ticket/26623 to u/mjo/ticket/26623ng
 Commit changed from 3c880885fbc910f21b76745dea1a141e205f6ce4 to f4a4d3488789021cb17169cf4134dd4787fa7568
 Status changed from needs_work to needs_review
If no one else, I have convinced myself that lowercasewithunderscores is the better option here.
Things got messy when I tried to revert the camelcase commit, so I wound up redoing everything. The new implementation is slightly different:
 I renamed the module from
cones
tocone_catalog
which fits better with the existing naming scheme for algebras, designs, etc. Someone pointed this out on the mailing list and it makes sense to me.  I got rid of the class, and put everything back into functions exposed at the top level. The whole module is now imported under the global name
cones
. The reason for not doing this in the first place is that you don't wantcones.<tab>
to show things likeZZ
andmatrix
that just happen to be imported into the module. After moving a few imports into the functions that use them, that's no longer an issue here.  Things are back to lowercasewithunderscores.
 The lazy import isn't needed any longer, because we're not at risk of importing a precreated object that will waste memory. Now it's just function names that do nothing unless you call them.
 All of the commit messages have been updated to reflect these changes.
comment:32 Changed 2 years ago by
 Status changed from needs_review to needs_work
Some minor comments.
 Your
INPUT
item all end with a period  You describe twice (after
INPUT
and afterOUTPUT
), when errors are being raised.  You misspelled
rearrangenent
at least once.  Please provide doctests for
_preprocess_args
. nonnegative_orthant
: Can you be more precise with the reference?
+ REFERENCES: + +  [BV2009]_

 Can you be more precise with the reference?
rearrangement
:"rearrange,"
(comma in quotes) again the references are rather unspecific except for
[Jeong2017]_
(is there any way to obtain[Jeong2017]_
?)  I would formulate the statement exactly the other way around. I guess figuring out the Lyapunov rank of the nonnegative orthant or a halfspace is the easy part.
+ Jeong's Corollary 5.2.4 [Jeong2017]_ states that if ``p`` is + either one or ``ambient_dim``, then the Lyapunov rank of the + rearrangement cone in ``ambient_dim`` dimensions is + ``ambient_dim``. Moreover for all other values of ``p``, its + Lyapunov rank is one::
 Isn't this clear from the definition anyway?
+ Jeong's Proposition 5.2.1 [Jeong2017]_ states that the rearrangenent + cone of order ``p`` is contained in the rearrangement cone of + order ``p + 1``::
So maybe just the following would suffice+ The rearrangement cone of order ``p`` is + by definition contained in the one of order ``p + 1``::
+ The generators for the rearrangement cone are given by [Jeong2017]_ + Theorem 5.2.3.
Maybe it's more useful for the reader to specify the generators concretely here.
schur
 Please provide a definition.
 Again the references would be more helpful with concrete information (such as sections etc.)
comment:33 Changed 2 years ago by
 Commit changed from f4a4d3488789021cb17169cf4134dd4787fa7568 to 42e9e2abb91c4b8c38b39588c5428d8643ec268a
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
ed90795  Trac #26623: reduce INPUT/OUTPUT documentation duplication.

d83f5e0  Trac #26623: add more documentation and examples for _preprocess_args().

06df9d9  Trac #26623: remove an unnecessary citation.

bde9302  Trac #26623: improve the ALGORITHM block for the rearrangement() cone.

6ceefa8  Trac #26623: clarify the example of Jeong's Corollary 5.2.4.

ab5ea7d  Trac #26623: fix a bound in a doctest (offbytwo from range()).

f54d45a  Trac #26623: improve the references for the rearrangement cone.

26618d4  Trac #26623: improve documentation for the schur() cone.

8034754  Trac #26623: remove two breaking spaces in LaTeX code.

42e9e2a  Trac #26623: remove periods from bullet list items.

comment:34 Changed 2 years ago by
 Status changed from needs_work to needs_review
Thanks for the feedback. I think I addressed all of your suggestions. I left each change separate for ease of review, but these commits can all be squashed eventually.
The only way I know of to get Juyoung's thesis is to email him and ask. I don't know of another published reference for the Lyapunov rank result, though.
comment:35 Changed 2 years ago by
Thanks. Overall this will be a nice improvement.
A few comments for now.
 I would suggest using commas here:
+globallyavailable ``cones`` prefix, to create some common cones: + + The nonnegative orthant, + the rearrangement cone of order ``p``, + the Schur cone, + the trivial cone.  globallyavailable ``cones`` prefix, to create some common cones:   The nonnegative orthant  The rearrangement cone of order ``p``  The Schur cone  The trivial cone
 The warning at the beginning should include mentioning, that any import will be imported at startup.
 If you lazy import
Cone
andrandom_cone
than the bots will be fine with the number of startup modules. Also this should be done anyway, shouldn't it?
comment:36 Changed 2 years ago by
 Commit changed from 42e9e2abb91c4b8c38b39588c5428d8643ec268a to 282a6943dc3e8e9172f2d96375a1f046907bfb77
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fdfd1e4  Trac #26623: add cones.<foo> shortcuts for common cones.

ffa1e9b  Trac #26623: add sage.geometry.cone_catalog to the documentation index.

282a694  Trac #26623: update sage.geometry.cone doctests to use the cone catalog.

comment:37 Changed 2 years ago by
Thanks, I added the commas, and expanded the warning at the beginning of the module.
As for the bots: I don't think they're complaining about Cone
or random_cone
:
 Both of those are loaded by
sage.geometry.all
anyway.
 Those import statements aren't executed until you run one of the functions in the
cone_catalog
module, so importing them is essentially "lazy" even if you're using sage as a library and not interactively.
Instead, I think the bots are pointing out that sage.geometry.cone_catalog
is now loaded into the global namespace as cones
, but that's almost unavoidable since that's the whole point. However, it is indeed slower than the lazy import of the class was, should you decide not to use it.
There may be some clever way to delay even that import, while still retaining the name "cones" along with its tabcompletion list. For example, if I give a toplevel variable name to the module, and then import that object lazily... e.g. this seems to work in cone_catalog.py
import sys # A python object that can be imported lazily with lazy_import(...). cones = sys.modules[__name__] def __dir__(): # Don't expose any global imports or variables to tabcompletion. return ['nonnegative_orthant', 'rearrangement', 'schur', 'trivial']
if I then do
lazy_import('sage.geometry.cone_catalog', 'cones')
in sage/geometry/all.py. But that's veering dangerously close to "too clever" to save a few microseconds in my opinion.
New commits:
fdfd1e4  Trac #26623: add cones.<foo> shortcuts for common cones.

ffa1e9b  Trac #26623: add sage.geometry.cone_catalog to the documentation index.

282a694  Trac #26623: update sage.geometry.cone doctests to use the cone catalog.

comment:38 followup: ↓ 40 Changed 2 years ago by
One could just do
lazy_import("sage.geometry", "cone_catalog", "cones")
I don't know if it's faster, but I don't see a disadvantage. Tab completion and everything still works.
comment:39 Changed 2 years ago by
 Commit changed from 282a6943dc3e8e9172f2d96375a1f046907bfb77 to e8fd707d791a9c09b65ba8722c505d7ff23d3738
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7567874  Trac #26623: add global entries for pending cone catalog citations.

39fd483  Trac #26623: add cones.<foo> shortcuts for common cones.

c6a1b3a  Trac #26623: add sage.geometry.cone_catalog to the documentation index.

e8fd707  Trac #26623: update sage.geometry.cone doctests to use the cone catalog.

comment:40 in reply to: ↑ 38 Changed 2 years ago by
Replying to ghkliem:
I don't know if it's faster, but I don't see a disadvantage. Tab completion and everything still works.
Thank you! I wasted a lot of time trying to figure out how to do that. I had even written a new lazy_import_module
function to implement the hack in my comment:37, but this is much simpler and will make the bots happy.
comment:41 followup: ↓ 43 Changed 2 years ago by
I failing doctest, due to the fact that there is a new submodule of geometry
.
File "src/sage/misc/dev_tools.py", line 164, in sage.misc.dev_tools.load_submodules Failed example: sage.misc.dev_tools.load_submodules(sage.geometry, "database$lattice") Expected: load sage.geometry.fan_isomorphism... succeeded load sage.geometry.hyperplane_arrangement.affine_subspace... succeeded ... load sage.geometry.riemannian_manifolds.surface3d_generators... succeeded Got: load sage.geometry.cone_catalog... succeeded load sage.geometry.fan_isomorphism... succeeded load sage.geometry.hyperbolic_space.hyperbolic_coercion... succeeded load sage.geometry.hyperbolic_space.hyperbolic_constants... succeeded load sage.geometry.hyperbolic_space.hyperbolic_geodesic... succeeded load sage.geometry.hyperbolic_space.hyperbolic_interface... succeeded load sage.geometry.hyperplane_arrangement.affine_subspace... succeeded load sage.geometry.hyperplane_arrangement.arrangement... succeeded load sage.geometry.hyperplane_arrangement.check_freeness... succeeded load sage.geometry.hyperplane_arrangement.library... succeeded load sage.geometry.hyperplane_arrangement.plot... succeeded load sage.geometry.newton_polygon... succeeded load sage.geometry.polyhedron.cdd_file_format... succeeded load sage.geometry.polyhedron.combinatorial_polyhedron.base... succeeded load sage.geometry.polyhedron.double_description... succeeded load sage.geometry.polyhedron.double_description_inhomogeneous... succeeded load sage.geometry.polyhedron.face... succeeded load sage.geometry.polyhedron.library... succeeded load sage.geometry.polyhedron.plot... succeeded load sage.geometry.pseudolines... succeeded load sage.geometry.ribbon_graph... succeeded load sage.geometry.riemannian_manifolds.parametrized_surface3d... succeeded load sage.geometry.riemannian_manifolds.surface3d_generators... succeeded
comment:42 Changed 2 years ago by
 Commit changed from e8fd707d791a9c09b65ba8722c505d7ff23d3738 to bfd757fc18fcd58ac86f232bc388e24127d19bf2
Branch pushed to git repo; I updated commit sha1. New commits:
bfd757f  Trac #26623: fix a doctest for sage.misc.dev_tools.load_submodules().

comment:43 in reply to: ↑ 41 Changed 2 years ago by
Replying to ghkliem:
I failing doctest, due to the fact that there is a new submodule of
geometry
.
Oof, fixed. The switch back to a lazy import caused that one.
comment:44 followup: ↓ 46 Changed 2 years ago by
Again many minor things.
 There is a tripple dash
+At the moment, only convex rational polyhedral cones are +supportedspecifically, those cones that can be built using the
 Instead of importing in each function, you could do something as
from sage.rings.all import ZZ as _ZZ
Don't know if that is better.  I don't know, if this is going to link correctly in the documentation:
+  ``lattice``  a :class:`ToricLattice` in which the cone will live
 I think we are supposed to use double quotation marks for error messages:
+ raise ValueError('lattice rank=%d and ambient_dim=%d ' + 'are incompatible' % (lattice.rank(), ambient_dim))
 Missing semicolons:
  ``ambient_dim``  (default: ``None``) the dimension of the  ambient space, a nonnegative integer    ``lattice``  (default: ``None``) the toric lattice in which  the cone will live +  ``ambient_dim``  (default: ``None``); the dimension of the + ambient space, a nonnegative integer + +  ``lattice``  (default: ``None``); the toric lattice in which + the cone will live
 Again I'm not sure that this will work in the documentation
+ A :class:`.ConvexRationalPolyhedralCone` living in ``lattice``
 I don't know if I would mention the errors after
OUTPUT
at all. Isn't this clear that thisOUTPUT
is given only if theINPUT
is as required and otherwise an error is raised? 
 I = matrix.identity(ZZ,ambient_dim) + I = matrix.identity(ZZ, ambient_dim)

 I = matrix.identity(ZZ,ambient_dim)  M = matrix.ones(ZZ,ambient_dim)  p*I + I = matrix.identity(ZZ, ambient_dim) + M = matrix.ones(ZZ, ambient_dim)  p*I
 The rearrangment cone will give nonsense, when
p
is not an integer. But I don't know if this is a problem.
comment:45 Changed 2 years ago by
 Commit changed from bfd757fc18fcd58ac86f232bc388e24127d19bf2 to ebb5a768f86362887238e82a29f3221974fd6237
Branch pushed to git repo; I updated commit sha1. New commits:
7660105  Trac #26623: use docstring conventions from the developer's guide.

f7e8af6  Trac #26623: add a few spaces to the cone catalog for readability.

e91d993  Trac #26623: make rearrangement cones of noninteger orders an error.

ebb5a76  Trac #26623: use doublequotes as first choice for string delimeters.

comment:46 in reply to: ↑ 44 Changed 2 years ago by
Replying to ghkliem:
Again many minor things.
 There is a tripple dash
This is actually the documented way to get sphinx to create an "em" dash in the HTML output. It's displaying correctly for me in the built docs.
 Instead of importing in each function, you could do something as
from sage.rings.all import ZZ as _ZZDon't know if that is better.
Clever =)
But, the extra imports are "free", so I don't think it's worth changing them around again at this point.
 I don't know, if this is going to link correctly in the documentation:
+  ``lattice``  a :class:`ToricLattice` in which the cone will live
You are right... ToricLattice
isn't a class.
 I think we are supposed to use double quotation marks for error messages:
Is that written down anywhere (for my future reference)? I don't care either way, I changed them to double quotes.
 Missing semicolons:
Hmm.. someone has updated the developer's guide without fixing the existing docstrings or mentioning it to anyone. I tried to conform to the new examples in the guide.
 Again I'm not sure that this will work in the documentation
+ A :class:`.ConvexRationalPolyhedralCone` living in ``lattice``
This one works!
 I don't know if I would mention the errors after
OUTPUT
at all. Isn't this clear that thisOUTPUT
is given only if theINPUT
is as required and otherwise an error is raised?
Other languages/projects have a standard location to document the exceptions that can be raised, but as far as I know, we don't. We probably should. I think it's nice to mention that a function can fail in the OUTPUT docs, just to be on the safe side in case the user skips the INPUT docs. But to me it feels like the relationship between the inputs belongs in the INPUT block. My mind could be changed on this, if everyone could agree to do it consistently.
 I = matrix.identity(ZZ,ambient_dim) + I = matrix.identity(ZZ, ambient_dim)
 I = matrix.identity(ZZ,ambient_dim)  M = matrix.ones(ZZ,ambient_dim)  p*I + I = matrix.identity(ZZ, ambient_dim) + M = matrix.ones(ZZ, ambient_dim)  p*I
Added spacing throughout.
 The rearrangement cone will give nonsense, when
p
is not an integer. But I don't know if this is a problem.
Thanks, I don't like that the function would silently return nonsense if you pass in e.g. p=1.5
I've added another error check to that cone.
comment:47 followup: ↓ 49 Changed 2 years ago by
 Reviewers set to Jonathan Kliem
Missing comma for the if ... then statement:
+ Jeong's Corollary 5.2.4 [Jeong2017]_ states that if `p = n1` in + an `n`dimensional ambient space then the Lyapunov rank of the
Otherwise, you can set it on positive review on my behalf.
comment:48 Changed 2 years ago by
 Commit changed from ebb5a768f86362887238e82a29f3221974fd6237 to b2aab916b145144f10f325161c551301ac34c351
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
24d97c0  Trac #26623: add cones.<foo> shortcuts for common cones.

1f454d1  Trac #26623: add sage.geometry.cone_catalog to the documentation index.

12ee26c  Trac #26623: update sage.geometry.cone doctests to use the cone catalog.

b2aab91  Trac #26623: fix a doctest for sage.misc.dev_tools.load_submodules().

comment:49 in reply to: ↑ 47 Changed 2 years ago by
 Status changed from needs_review to positive_review
Replying to ghkliem:
Missing comma for the if ... then statement:
...
Otherwise, you can set it on positive review on my behalf.
Thanks again. I squashed this and the other recent commits into the main one. It will be nice to have this off my todo list.
comment:50 Changed 2 years ago by
 Milestone changed from sage8.5 to sage9.1
comment:51 Changed 2 years ago by
 Branch changed from u/mjo/ticket/26623ng to b2aab916b145144f10f325161c551301ac34c351
 Resolution set to fixed
 Status changed from positive_review to closed
They certainly should take a lattice argument! From a toric perspective it is probably more natural to have these as methods of lattices, i.e. something like
although of course this makes it impossible to use
ZZ^n
. And perhaps even more natural is to have not cones, but associated toric varieties which are already available: