Opened 8 years ago

Closed 4 years ago

#11284 closed defect (fixed)

Document all options of Poset.show and Poset.plot

Reported by: nthiery Owned by: sage-combinat
Priority: major Milestone: sage-6.5
Component: combinatorics Keywords: posets, documentation, days30
Cc: sage-combinat, jmantysalo, ncohen Merged in:
Authors: Nathann Cohen Reviewers: Jori Mäntysalo
Report Upstream: N/A Work issues:
Branch: 329de0c (Commits) Commit: 329de0c33099d582cede2d26c307cf3da4d7ae16
Dependencies: Stopgaps:

Description (last modified by ncohen)

This branch adds documentation to FinitePoset.show and FinitePoset.plot. It also lets one use a parameter of plot when calling show.

It also removes the show and plot methods from HasseDiagram: those two functions were never used anywhere as FinitePoset.plot calls DiGraph.plot directly. They also accepted parameters that were never used in the code.

As HasseDiagram is a internal class (not meant for users) no deprecation was added.

Nathann

P.S.: while it seems that the code removes arguments from show and plot it is not the case: these arguments are handled as they should by the subcall to the graph plot function.

Change History (21)

comment:1 Changed 8 years ago by nthiery

  • Keywords days30 added

comment:2 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:6 Changed 4 years ago by jmantysalo

  • Cc jmantysalo added

comment:7 Changed 4 years ago by jmantysalo

Relating to this: Where is default pink color defined? For example

G=DiGraph({0:[1]})
G.show(vertex_colors='pink')
G.show(vertex_colors={'#ff00ff':[0]})

shows kind of a bug: color of element 1 changes from pink to blue when setting color of 0 to red.

Examples to show() could be something like

P.show(vertex_colors={'red':P.maximal_elements()})

or

for c in P.maximal_chains():
    P.show(vertex_colors={'red':c})

Where P would be suitable poset.

comment:8 Changed 4 years ago by jmantysalo

  • Cc ncohen added

Comment fron ncohen on #17477: "The problem comes from HasseDiagram.plot. This argument [i.e. label_font_size], and two others, are totally ignored. Also, it seems that this HasseDiagram.plot function is never used: Poset.plot calls GenericGraph.plot directly."

comment:9 Changed 4 years ago by ncohen

(running tests for public/11284 at the moment)

comment:10 Changed 4 years ago by ncohen

  • Authors set to Nathann Cohen
  • Branch set to public/11284
  • Commit set to 329de0c33099d582cede2d26c307cf3da4d7ae16
  • Description modified (diff)
  • Status changed from new to needs_review

Ready for review !

Nathann


New commits:

329de0ctrac #11284: Document all options of Poset.show and Poset.plot

comment:11 Changed 4 years ago by ncohen

  • Description modified (diff)

comment:12 Changed 4 years ago by jmantysalo

Docstring "Displays the Hasse diagram of the poset." should be "Display the Hasse diagram of the poset." according to PEP whose number I don't remember.

comment:13 Changed 4 years ago by ncohen

Okay. Well, if this is the only comment you have to make to give this patch a positive review I will add a commit, otherwise I will do it along with the other remarks if you do not mind :-P

Nathann

comment:14 follow-up: Changed 4 years ago by jmantysalo

I still don't see how the user can find figsize-option. And IMO it is one of most important --- it is good to make smaller with poset of ~7 elements, almost necessary to make bigger with poset of 15 elements.

cover_labels put labels over the line, not near the line --- but this is place for another ticket.

Maybe element_labels could have explanation saying that it must be injective. And that cover_labels may be non-injective.

Hmm... how about some nice example of colors? Poset with top element and color by möbius function from top to element? Or a lattice where deleting some elements would make it non-lattice and those drawn as red? But there is no easy way to make graphics in documentation, so maybe no.

Thinking... Any opinions from others?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by ncohen

Yo !

I still don't see how the user can find figsize-option. And IMO it is one of most important

True. Actually, it can be found in the doc of Poset.show which mentions the doc of Graphics.show, where it appears. Not exactly as trivial as it should.

cover_labels put labels over the line, not near the line --- but this is place for another ticket.

For this kind of stuff, I would say that people will try and see for themselves. The drawing is better than a long explanation :-P

Maybe element_labels could have explanation saying that it must be injective. And that cover_labels may be non-injective.

Of changing the code to support non-injective labellings ! That is the most proper way to solve it.

Hmm... how about some nice example of colors? Poset with top element and color by möbius function from top to element? Or a lattice where deleting some elements would make it non-lattice and those drawn as red? But there is no easy way to make graphics in documentation, so maybe no.

You can still add an example, even if the code figure does not appear... It may change someday :-)

Nathann

comment:16 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by jmantysalo

Replying to ncohen:

I still don't see how the user can find figsize-option. And IMO it is one of most important

True. Actually, it can be found in the doc of Poset.show which mentions the doc of Graphics.show, where it appears. Not exactly as trivial as it should.

?? P._hasse_diagram.show? does not show it. I think that user must do at least two jumps to find it.

Maybe element_labels could have explanation saying that it must be injective. And that cover_labels may be non-injective.

Of changing the code to support non-injective labellings ! That is the most proper way to solve it.

Of course.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by ncohen

True. Actually, it can be found in the doc of Poset.show which mentions the doc of Graphics.show, where it appears. Not exactly as trivial as it should.

?? P._hasse_diagram.show? does not show it. I think that user must do at least two jumps to find it.

Nonono, not P._hasse_diagram.show. This function does not even exist anymore when this branch is applied. I was talking of P.show? after this branch is applied. Though it is easier to read if you browse it through the html pages.

Nathann

comment:18 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by jmantysalo

  • Milestone changed from sage-6.4 to sage-6.5
  • Reviewers set to Jori Mäntysalo

Replying to ncohen:

Nonono, not P._hasse_diagram.show. This function does not even exist anymore when this branch is applied.

It does, because Hasse diagram is DiGraph. Just like for example P._hasse_diagram.is_connected() is available.

But in any case, you are right. I was looking Graph.show?, not Graphics.show?.

You can mark this as positive review if you want. If not, I'll get back to this on monday and think about adding some examples.

comment:19 in reply to: ↑ 18 Changed 4 years ago by ncohen

  • Status changed from needs_review to positive_review

Yoooooo !

It does, because Hasse diagram is DiGraph. Just like for example P._hasse_diagram.is_connected() is available.

Oops right sorry :-D

In this case, however, you should also see a mention of Graphics.show:

http://www.sagemath.org/doc/reference/graphs/sage/graphs/generic_graph.html#sage.graphs.generic_graph.GenericGraph.show

You can mark this as positive review if you want. If not, I'll get back to this on monday and think about adding some examples.

Works for me, thanks !

Nathann

comment:20 Changed 4 years ago by jmantysalo

No more comments for now because of #17498. Maybe I'll get back to this after it has been applied.

comment:21 Changed 4 years ago by vbraun

  • Branch changed from public/11284 to 329de0c33099d582cede2d26c307cf3da4d7ae16
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.