Opened 6 years ago
Closed 6 years ago
#19831 closed enhancement (fixed)
Add random_element() for cones
Reported by:  mjo  Owned by:  

Priority:  major  Milestone:  sage7.0 
Component:  geometry  Keywords:  
Cc:  novoselt  Merged in:  
Authors:  Michael Orlitzky  Reviewers:  Andrey Novoseltsev 
Report Upstream:  N/A  Work issues:  
Branch:  690071f (Commits, GitHub, GitLab)  Commit:  690071f972423a123b07b0b7d6051ec25e335cb7 
Dependencies:  Stopgaps: 
Description
For testing and demonstration it's sometimes useful to be able to get "any" element of a cone. Other structures use random_element
for the same purpose.
Change History (12)
comment:1 Changed 6 years ago by
 Branch set to u/mjo/ticket/19831
 Cc novoselt added
 Commit set to c2a6310f5ca9c7b5fe0a4bf8b01ddede8a421a1b
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 6 years ago by
In the end comment "coercion" is not the right word  it is what happens automatically when mixing objects. Explicit one is "conversion".
I am a bit unsure about rationality. Seems like returning a random element of the lattice would be more natural and those who want can then scale by a rational or even real factor. Or there could be an optional parameter for this, defaulting to all integral weights?
comment:3 Changed 6 years ago by
 Commit changed from c2a6310f5ca9c7b5fe0a4bf8b01ddede8a421a1b to c2ff0460b15f2d4175be60fc34c706408d90e8a0
Branch pushed to git repo; I updated commit sha1. New commits:
c2ff046  Trac #19831: change "coercion" to "conversion" in a comment (review).

comment:4 in reply to: ↑ 2 ; followup: ↓ 5 Changed 6 years ago by
Replying to novoselt:
I am a bit unsure about rationality. Seems like returning a random element of the lattice would be more natural and those who want can then scale by a rational or even real factor.
This wouldn't be too bad but it makes all of my uses of it ugly. Each invocation is K.random_element()
versus QQ.random_element().abs()*vector(K.random_element())
.
I'm using rationals because some of the properties I'm testing involve norms (say, in a denominator) where you really want to test a value between zero and one.
Or there could be an optional parameter for this, defaulting to all integral weights?
This would be no problem, but how should the return type work? If we use integral weights we could return a lattice element but with any other field it would have to be a vector.
comment:5 in reply to: ↑ 4 ; followup: ↓ 7 Changed 6 years ago by
Replying to mjo:
Replying to novoselt:
Or there could be an optional parameter for this, defaulting to all integral weights?
This would be no problem, but how should the return type work? If we use integral weights we could return a lattice element but with any other field it would have to be a vector.
I definitely would like to get an appropriate lattice element for (default) integral weights and with anything else a vector is fine, that's consistent with other places that attempt to work with lattices but fall back to generic vectors/modules when this is not possible/implemented.
comment:6 Changed 6 years ago by
 Commit changed from c2ff0460b15f2d4175be60fc34c706408d90e8a0 to 1418c70e3d1f4b1bb5ab356b14feda8bcc85ec87
Branch pushed to git repo; I updated commit sha1. New commits:
1418c70  Trac #19831: add "ring" parameter to random_element() for cones.

comment:7 in reply to: ↑ 5 ; followup: ↓ 8 Changed 6 years ago by
Replying to novoselt:
I definitely would like to get an appropriate lattice element for (default) integral weights and with anything else a vector is fine, that's consistent with other places that attempt to work with lattices but fall back to generic vectors/modules when this is not possible/implemented.
That works for me; it only adds two characters to most invocations.
I decided to limit the possible choices to ZZ
/QQ
for a few reasons. First, for "nonnegative" to make any sense, we need to have a subthing of RR
. But RR.is_subring(RR)
doesn't work, and more importantly, the resulting elements don't live in either the cone's lattice or its ambient vector space. That means matrix multiplication (on the ambient space) might crash even though K.contains(x)
still returns True
.
Maybe it makes more sense to have e.g. latticial=True
instead of ring=ZZ
when there are only two choices (ambient lattice or ambient vector space)?
comment:8 in reply to: ↑ 7 Changed 6 years ago by
Replying to mjo:
Maybe it makes more sense to have e.g.
latticial=True
instead ofring=ZZ
when there are only two choices (ambient lattice or ambient vector space)?
That's the first time I hear such a word ;) In the toric geometry cones live in real vector spaces. The fact that Sage associates a rational and not real one to free modules is just the way how Sage is, in part due to technical reasons of working with reals on computers, I guess. In any case there is no reason why eventually we can't accept any kind of real field with different precision or perhaps some other subrings. So I am more in favour of NotImplementedError
rather than ValueError
since RR
, RDF
etc are very sensible choices.
comment:9 Changed 6 years ago by
 Commit changed from 1418c70e3d1f4b1bb5ab356b14feda8bcc85ec87 to 690071f972423a123b07b0b7d6051ec25e335cb7
Branch pushed to git repo; I updated commit sha1. New commits:
690071f  Trac #19831: change ValueError to NotImplementedError in random_element().

comment:10 Changed 6 years ago by
I made up the word, but it sounds very official =P
NotImplementedError
is fine with me. My cones are real, too, but nothing works when we use floating point numbers.
comment:11 Changed 6 years ago by
 Reviewers set to Andrey Novoseltsev
 Status changed from needs_review to positive_review
Thank you!
comment:12 Changed 6 years ago by
 Branch changed from u/mjo/ticket/19831 to 690071f972423a123b07b0b7d6051ec25e335cb7
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #19831: Add random_element() method for convex cones.