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: sage-7.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:

Status badges

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 mjo

  • Authors set to Michael Orlitzky
  • Branch set to u/mjo/ticket/19831
  • Cc novoselt added
  • Commit set to c2a6310f5ca9c7b5fe0a4bf8b01ddede8a421a1b
  • Status changed from new to needs_review

New commits:

c2a6310Trac #19831: Add random_element() method for convex cones.

comment:2 follow-up: Changed 6 years ago by novoselt

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 git

  • Commit changed from c2a6310f5ca9c7b5fe0a4bf8b01ddede8a421a1b to c2ff0460b15f2d4175be60fc34c706408d90e8a0

Branch pushed to git repo; I updated commit sha1. New commits:

c2ff046Trac #19831: change "coercion" to "conversion" in a comment (review).

comment:4 in reply to: ↑ 2 ; follow-up: Changed 6 years ago by mjo

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.

Last edited 6 years ago by mjo (previous) (diff)

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by novoselt

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 git

  • Commit changed from c2ff0460b15f2d4175be60fc34c706408d90e8a0 to 1418c70e3d1f4b1bb5ab356b14feda8bcc85ec87

Branch pushed to git repo; I updated commit sha1. New commits:

1418c70Trac #19831: add "ring" parameter to random_element() for cones.

comment:7 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by mjo

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 novoselt

Replying to mjo:

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)?

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 git

  • Commit changed from 1418c70e3d1f4b1bb5ab356b14feda8bcc85ec87 to 690071f972423a123b07b0b7d6051ec25e335cb7

Branch pushed to git repo; I updated commit sha1. New commits:

690071fTrac #19831: change ValueError to NotImplementedError in random_element().

comment:10 Changed 6 years ago by mjo

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 novoselt

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to positive_review

Thank you!

comment:12 Changed 6 years ago by vbraun

  • Branch changed from u/mjo/ticket/19831 to 690071f972423a123b07b0b7d6051ec25e335cb7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.