Opened 3 years ago

Closed 2 years ago

#22572 closed enhancement (fixed)

Add a thematic tutorial on the polyhedron class

Reported by: jipilab Owned by:
Priority: major Milestone: sage-8.3
Component: geometry Keywords: days84, geometry, tutorial, days88, days93, IMA-PolyGeom
Cc: moritz, mkoeppe, vdelecroix, tmonteil, chapoton, mmasdeu, tscrim, mforets, SimonKing, novoselt, vklein Merged in:
Authors: Jean-Philippe Labbé, Vincent Delecroix Reviewers: Moritz Firsching
Report Upstream: N/A Work issues:
Branch: b74ae85 (Commits) Commit: b74ae85343beeaefb536efe7d3c0fd0402d62217
Dependencies: #22558, #22565 Stopgaps:

Description (last modified by jipilab)

This ticket adds a thorough tutorial surrounding the polyhedron class.

The tutorial was elaborated during the Sage Days 84 where all participants gave several significant inputs about the tutorial and its content.

FOLLOW UPS:

  • Include the following methods in the tutorial in a follow-up ticket: #22574, #22575
  • #17215: Implement the normal_cone of a face
  • #24837: Improve the output of repr_pretty

Change History (39)

comment:1 follow-up: Changed 3 years ago by jipilab

  • Cc SimonKing added

What is the username of Vincent Klein?

I should try to push a version complete "draft version" very soon. Not much is left.

Meanwhile, a question: is it possible that a ref block in rst can not include math mode?

comment:2 Changed 3 years ago by novoselt

  • Cc novoselt added

comment:3 Changed 3 years ago by jipilab

  • Dependencies set to #22605, #22498, #22565

comment:4 Changed 3 years ago by jipilab

  • Branch set to u/jipilab/22572

comment:5 Changed 3 years ago by jipilab

  • Commit set to fdcb144a5d2847bda231ca415ba5969162a026da
  • Dependencies changed from #22605, #22498, #22565 to #22605, #22498, #22565, #22522, #22622, #22558, #22546, #22574, #22575

Last 10 new commits:

4b9192dAdded one more file
5d4faf5duplicate label
7213306Edits in titles
81d71bcadded some missing methods
a1ad1afAdded tips and some more methods
7fcfb04Reorder alphabetically in visualization
46f48faAdded description of is_ methods
79bb4ceCorrected a link and added Ehrhart poly
e2aa8a8Repaired references to objects
fdcb144Added ref to Perles

comment:6 in reply to: ↑ 1 ; follow-up: Changed 3 years ago by mforets

Replying to jipilab:

What is the username of Vincent Klein?

It is vklein.

I should try to push a version complete "draft version" very soon. Not much is left.

Are you looking for a reviewer for this ticket?

Meanwhile, a question: is it possible that a ref block in rst can not include math mode?

Hmm, I didn't understand :/

comment:7 in reply to: ↑ 6 Changed 3 years ago by jipilab

Thanks Marcelo!

My question refers to the fact that I make references to the base classes using :ref: and in this block, the math does not appear, like QQ and ZZ. I do not know why...

As for reviewers, you are welcome to review the ticket! I am not completely done yet though... That's why I left it as new. I still want to put some more work in there, but I thought that it is good if people can have a look at it already!

comment:8 Changed 3 years ago by jipilab

  • Cc vklein added

comment:9 Changed 3 years ago by jipilab

  • Dependencies changed from #22605, #22498, #22565, #22522, #22622, #22558, #22546, #22574, #22575 to #20887, #22605, #22498, #22565, #22522, #22622, #22558, #22546, #22574, #22575

comment:10 Changed 3 years ago by jipilab

  • Dependencies changed from #20887, #22605, #22498, #22565, #22522, #22622, #22558, #22546, #22574, #22575 to #20887, #22605, #22498, #22565, #22522, #22622, #22558, #22546,
  • Description modified (diff)

Since the tickets #22574, #22575 are not quite ready yet and I'd like this ticket done soon, I put them in a forthcoming ticket to do later.(!)

comment:11 Changed 3 years ago by jipilab

  • Keywords days88 added

comment:12 Changed 3 years ago by vdelecroix

you have to remove trailing whitespaces

comment:13 Changed 3 years ago by vdelecroix

I would not order a tutorial by method. This is what the reference manual is about. You should find a "fil rouge". I think the best way to proceed is to have concrete big problems and show how to solve it in Sage step by step.

Furthermore, I think that it is very good to have exercices in thematic tutorials!

EDIT: there are actually a lot of different files:

  • is_this_polyhedron.rst
  • lectures.rst
  • new_from_old.rst
  • related_objects.rst
  • tips.rst
  • visualization.rst

I think it is bad to put these rather generic names. Geometry is not just about polyhedra...

Last edited 3 years ago by vdelecroix (previous) (diff)

comment:14 Changed 3 years ago by vdelecroix

  1. Change the title Tutorials for the Geometry module of Sage into Polyhedra in the file geometry.rst
  1. At the very top of the main thematic tutorial page there is a nice distinction between quickrefs, primer and tutorial. It would be good to tag the different files you wrote accordingly.

comment:15 Changed 3 years ago by vdelecroix

  • Branch changed from u/jipilab/22572 to public/22572
  • Commit changed from fdcb144a5d2847bda231ca415ba5969162a026da to d2dd9f72cb398f9e16b5c45ab1c3d9c1e3516966
  • Dependencies changed from #20887, #22605, #22498, #22565, #22522, #22622, #22558, #22546, to #22558, #22565
  • Milestone changed from sage-7.6 to sage-8.1

New commits:

01bc98522572: first draft of polyhedra tutorial
d2dd9f722572: revamp polyhedra tutorials

comment:16 Changed 3 years ago by vdelecroix

I rebased everything on 8.1.beta3 (folding all your commits in 1). The three documents is_this_polyhedron.rst, new_from_old.rst and related_objects.rst are now folded in a unique polyhedra_quickref.rst.

The organization of the quickref should be discussed...

comment:17 Changed 3 years ago by jipilab

  • Description modified (diff)

I added some todos that I observed while making the tutorial.

comment:18 Changed 2 years ago by jipilab

  • Branch changed from public/22572 to public/22572v8.2b5
  • Commit changed from d2dd9f72cb398f9e16b5c45ab1c3d9c1e3516966 to 5a09b69e7b0b48072c88766365229f1ca1c1831d

New commits:

e1904c622572: first draft of polyhedra tutorial
5a09b6922572: revamp polyhedra tutorials

comment:19 Changed 2 years ago by git

  • Commit changed from 5a09b69e7b0b48072c88766365229f1ca1c1831d to db5e4135aaca2b70fa6ad9ae0981f9c89df52065

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

db5e413Updated to 8.2.b5, fixed many errors

comment:20 Changed 2 years ago by jipilab

  • Authors changed from Jean-Philippe Labbé to Jean-Philippe Labbé, Vincent Delecroix
  • Description modified (diff)
  • Keywords days93 added
  • Milestone changed from sage-8.1 to sage-8.2
  • Status changed from new to needs_review

comment:21 Changed 2 years ago by git

  • Commit changed from db5e4135aaca2b70fa6ad9ae0981f9c89df52065 to a7b78dda33eacf7ed91d08c78cd549baf56e4ae2

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

a7b78ddAdded a ref to the handbook

comment:22 Changed 2 years ago by git

  • Commit changed from a7b78dda33eacf7ed91d08c78cd549baf56e4ae2 to 31edde6af8a893ddd9ccb3bfd873938a0996b412

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

31edde6Completed a comment

comment:23 Changed 2 years ago by vdelecroix

You should not use a generic name for your file like geometry/lectures.rst. Your tutorial is specific on polytopes. Note that there is moreover a polytutorial.rst. Perhaps polytope_quick_tutorial.rst for the latter and polytope_tutorial.rst (that way all of them starts with polytope that helps)?

comment:24 Changed 2 years ago by vdelecroix

Some sided comment:

  • :math:`\mathbb{R}^d` could be simplified to `\mathbb{R}^d` (the default sphinx directive for backstick is math)
  • `\mathbb{R}^d` could be simplifed in `\RR^d` (sage comes with some predefined macro)

comment:25 follow-up: Changed 2 years ago by sdrewitz

in lectures.rst at the beginning of the first lecture right after the second displayed equation, do you need to ask for the \lambda_i to be \geq 0 ?

comment:26 Changed 2 years ago by git

  • Commit changed from 31edde6af8a893ddd9ccb3bfd873938a0996b412 to bd6c356e98464b7fa661e4df85a2a2b7e1bd219f

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

231cd56deleted trailing whitespace
f685c64fixed some typos in lectures.rst
7f5e125fixed typos in polyhedra_quickref.rst
bd6c356LateX -> LaTeX in polytope_tikz.rst

comment:27 in reply to: ↑ 25 Changed 2 years ago by jipilab

Replying to sdrewitz:

in lectures.rst at the beginning of the first lecture right after the second displayed equation, do you need to ask for the \lambda_i to be \geq 0 ?

Yes! Thanks for spotting this!

comment:28 Changed 2 years ago by git

  • Commit changed from bd6c356e98464b7fa661e4df85a2a2b7e1bd219f to d526f70d1425078e9b617704ec888bc89ddcb0d5

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

b1ce45bSeveral other corrections
d526f70renamed tutorial files

comment:29 follow-up: Changed 2 years ago by jipilab

I just pushed the suggested changes.

One remark: I prefer how the \mathbf{R} appears compared to \RR.

comment:30 in reply to: ↑ 29 Changed 2 years ago by vdelecroix

Replying to jipilab:

I just pushed the suggested changes.

Great :-)

One remark: I prefer how the \mathbf{R} appears compared to \RR.

Then propose to change it globally in Sage? This is just a macro. It is better to have it consistent accross documentation rather than good looking on your tutorial.

comment:31 Changed 2 years ago by git

  • Commit changed from d526f70d1425078e9b617704ec888bc89ddcb0d5 to d7896f8d78766bd364abfc06dd57c9ebc84c9975

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

d7896f8Merge branch 'develop' into 22572

comment:32 Changed 2 years ago by git

  • Commit changed from d7896f8d78766bd364abfc06dd57c9ebc84c9975 to 597b802e6d0aead26d680870d43052cf09e9b2f9

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

597b802Merge branch sage8.2.rc1 into 22572

comment:33 Changed 2 years ago by moritz

  • Reviewers set to Moritz Firsching
  • Status changed from needs_review to needs_work

Thanks for the tutorial!

I only have a few remarks/corrections

  • has the ambient space --> as the ambient space
  • should the explanation for "is_compact" perhaps mention the word "bounded"?
  • add a link for "composite_field" (and change the word to "composite_fields")
  • the reason why we cant do computations in the symbolic ring is given as because it is "not exact". Perhaps we could simply state that it is not implemented. (Computations in RDF are also "not exact")
  • access to vertices ... --> access vertices ...
  • remove the part "clearly (!)" (or is this a joke I don't get?)
  • in the sage_input example, I suggest to change whats there to
sage: Cube = polytopes.cube()
sage: TCube = Cube.truncation().dilation(1/2)
sage: sage_input(TCube)

That is add "dilation" to avoid "QQ(1)" many times, which doesn't look pretty.

comment:34 Changed 2 years ago by git

  • Commit changed from 597b802e6d0aead26d680870d43052cf09e9b2f9 to b74ae85343beeaefb536efe7d3c0fd0402d62217

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

eee533aCorrections from review
b74ae85Made tests pass

comment:35 Changed 2 years ago by jipilab

  • Status changed from needs_work to needs_review

Salut Moritz,

Thanks a lot for the review. I made the corrections you suggested.

Hopefully, the bot likes it.

comment:36 Changed 2 years ago by moritz

  • Status changed from needs_review to positive_review

comment:37 Changed 2 years ago by jipilab

  • Keywords IMA-PolyGeom added

comment:38 Changed 2 years ago by jipilab

  • Milestone changed from sage-8.2 to sage-8.3

comment:39 Changed 2 years ago by vbraun

  • Branch changed from public/22572v8.2b5 to b74ae85343beeaefb536efe7d3c0fd0402d62217
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.