Opened 9 months ago

Closed 2 months ago

#28247 closed enhancement (fixed)

Parametrize the cube/hypercube functions in the library of polytopes

Reported by: selia Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords: polytopes, cube, days100
Cc: jipilab, nailuj, gh-kliem Merged in:
Authors: Sophia Elia Reviewers: Jean-Philippe Labbé, Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: 6da26d9 (Commits) Commit: 6da26d9b9020fd02a2ed43163e2c4f3550bf3e92
Dependencies: Stopgaps:

Description

Currently, the polytopes.cube and polytopes.hypercubes use vertex coordinates +/-1. It would be nice to have the 0/1 cube and the 0/1 hypercube. This ticket adds them to the library of polytopes.

Change History (29)

comment:1 Changed 9 months ago by gh-kliem

  • Cc gh-kliem added

comment:2 follow-up: Changed 9 months ago by vdelecroix

Don't you want to parametrize the already existing cube/hypercube functions? Given x and y and more generally x1,y1, ..., xd,yd you can create the polytope

[x1,y1] x [x2,y2] x ... x [xd,yd]

comment:3 in reply to: ↑ 2 Changed 9 months ago by jipilab

Replying to vdelecroix:

Don't you want to parametrize the already existing cube/hypercube functions? Given x and y and more generally x1,y1, ..., xd,yd you can create the polytope

[x1,y1] x [x2,y2] x ... x [xd,yd]

Yes, yes, that's the idea; making it inside cube and hypercube was the intended plan.

I am not sure whether to also include:

[x1,y1] x [x2,y2] x ... x [xd,yd]

as well in there. One could try.

There are many ways to interpret x above. There is polytopes.zonotope which does:

sage: corner = vector([1,2,3,4])
sage: directions = diagonal_matrix([10,9,8,7]).columns()
sage: z = polytopes.zonotope(directions).translation(corner)
sage: z.vertices_list()
[[1, 2, 3, 4],
 [1, 2, 3, 11],
 [1, 2, 11, 4],
 [1, 2, 11, 11],
 [1, 11, 3, 4],
 [1, 11, 3, 11],
 [1, 11, 11, 4],
 [1, 11, 11, 11],
 [11, 2, 3, 4],
 [11, 2, 3, 11],
 [11, 2, 11, 4],
 [11, 2, 11, 11],
 [11, 11, 3, 4],
 [11, 11, 3, 11],
 [11, 11, 11, 4],
 [11, 11, 11, 11]]
sage: hc = polytopes.hypercube(4)
sage: z.is_combinatorially_isomorphic(hc)
True

That said, I see that it might be nice to have this directly possible inside of cube and hypercube...

comment:4 Changed 3 months ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:5 Changed 3 months ago by selia

  • Branch set to public/28247
  • Commit set to 301b3a1bed92baed96171f12e71e58c1de01f8dd

New commits:

301b3a1first version

comment:6 Changed 3 months ago by git

  • Commit changed from 301b3a1bed92baed96171f12e71e58c1de01f8dd to c6ac53d6e5d89e14091f6f1a5fc10c2020a030cc

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

c6ac53dfixing doctests

comment:7 Changed 3 months ago by git

  • Commit changed from c6ac53d6e5d89e14091f6f1a5fc10c2020a030cc to f6bcc4ebe49edfc9d78968d479f20e1ea9441069

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

f6bcc4epyflakes

comment:8 Changed 3 months ago by git

  • Commit changed from f6bcc4ebe49edfc9d78968d479f20e1ea9441069 to 17d8398d783b46e72c1017cb90c796556b3e8caa

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

17d8398edits

comment:9 Changed 3 months ago by selia

  • Status changed from new to needs_review

comment:10 Changed 3 months ago by selia

  • Authors set to Sophia Elia

comment:11 Changed 3 months ago by gh-kliem

Please make the zero-one string consistent.

comment:12 Changed 3 months ago by gh-kliem

Looks mostly good. Some comments:

  • In the docstring of cube remove empty line and unindent the output of cc.vertices_list().
  • three intervals of length 2: Do not specify the length of interval.
  • Instead of vectors in \RR{\text{dim}} one could specify the length or dimension of the vectors, to account for the polyhedron to be constructed in ZZ or QQ`.
  • I would maybe move the test of the error the dimension of the hypercube must match the number of intervals to tests. I don't think it is very interesting for the user.
  • -        elif isinstance(intervals,str):
    +        elif isinstance(intervals, str):
    
  • Missing parenthesis
    -            cp = list(itertools.product(*intervals)
    +            cp = list(itertools.product(*intervals))
    

comment:13 Changed 3 months ago by gh-kliem

  • Status changed from needs_review to needs_work

comment:14 Changed 3 months ago by git

  • Commit changed from 17d8398d783b46e72c1017cb90c796556b3e8caa to 8967a0f356bcf51adacadebec8274dd34b7a3e97

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

8967a0fadding edits

comment:15 Changed 3 months ago by git

  • Commit changed from 8967a0f356bcf51adacadebec8274dd34b7a3e97 to a9d7a714184009d71de654b8519919dde11381e4

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

a9d7a71fixing variable name

comment:16 Changed 3 months ago by selia

  • Status changed from needs_work to needs_review

comment:17 Changed 3 months ago by gh-kliem

Some minor commens:

  • Please update the description to include arbitrary intervals.
  • The INPUT block should also mention the behavior for the input None.
  • Instead of list/tuple/iterable of length I would just say list, especially as an iterable does not even work in general (len does not work).
  • A string should be written as code:
    -          - 'zero_one' -- (string). Return the `0/1`-cube.
    +          - ``'zero_one'`` -- Return the `0/1`-cube.
    
  • Usually INPUT blocks do not have periods, but I don't know if you should change this for this method now.

comment:18 Changed 3 months ago by git

  • Commit changed from a9d7a714184009d71de654b8519919dde11381e4 to 696dbe01c1d84f662fc8362b898cb7fc4d250460

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

696dbe0fixing comments

comment:19 Changed 3 months ago by selia

@gh-kliem -

Please update the description to include arbitrary intervals.

Can you elaborate on what you mean by including arbitrary intervals?

comment:20 Changed 3 months ago by gh-kliem

The code is more general than the description of the ticket. E.g. you also add the 0/2-cube.

It would be nice if the description reflects that one can now get the [a_1,b_1],…,[a_n,b_n] Cube.

comment:21 Changed 3 months ago by gh-kliem

See the very first comment of the ticket. That should somehow go up in the description.

comment:22 Changed 3 months ago by selia

  • Summary changed from Add the 0/1-hypercube to the library of polytopes to Parametrize the cube/hypercube functions in the library of polytopes

comment:23 Changed 3 months ago by gh-kliem

I think you accidentially commited this:

 .. csv-table::
-    :class: contentstable
+ )   :class: contentstable
     :widths: 30
     :delim: |

comment:24 Changed 3 months ago by git

  • Commit changed from 696dbe01c1d84f662fc8362b898cb7fc4d250460 to ce4627f094a43a54725c7374f44947e7c91ca067

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

ce4627fremoving parenthesis

comment:25 Changed 3 months ago by gh-kliem

  • Status changed from needs_review to needs_work

There is a double colon after EXAMPLES cube, followed by another comment. This makes building the documentation fail. Please remove it:

-         EXAMPLES::
+         EXAMPLES:
 
          Return the `\pm 1`-cube::

comment:26 Changed 3 months ago by git

  • Commit changed from ce4627f094a43a54725c7374f44947e7c91ca067 to 6da26d9b9020fd02a2ed43163e2c4f3550bf3e92

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

6da26d9removing colon

comment:27 Changed 3 months ago by selia

  • Status changed from needs_work to needs_review

comment:28 Changed 3 months ago by gh-kliem

  • Reviewers set to Jean-Philippe Labbé, Jonathan Kliem
  • Status changed from needs_review to positive_review

LGTM.

@JP: I put you as reviewer, I hope that is ok.

comment:29 Changed 2 months ago by vbraun

  • Branch changed from public/28247 to 6da26d9b9020fd02a2ed43163e2c4f3550bf3e92
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.