Opened 10 years ago
Closed 6 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 )
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 10 years ago by
- Keywords days30 added
comment:2 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:3 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:4 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:5 Changed 6 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:6 Changed 6 years ago by
- Cc jmantysalo added
comment:7 Changed 6 years ago by
comment:8 Changed 6 years ago by
- 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 6 years ago by
(running tests for public/11284 at the moment)
comment:10 Changed 6 years ago by
- 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:
329de0c | trac #11284: Document all options of Poset.show and Poset.plot
|
comment:11 Changed 6 years ago by
- Description modified (diff)
comment:12 Changed 6 years ago by
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 6 years ago by
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: ↓ 15 Changed 6 years ago by
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: ↓ 16 Changed 6 years ago by
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 thatcover_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: ↓ 17 Changed 6 years ago by
Replying to ncohen:
I still don't see how the user can find
figsize
-option. And IMO it is one of most importantTrue. 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 thatcover_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: ↓ 18 Changed 6 years ago by
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: ↓ 19 Changed 6 years ago by
- 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 6 years ago by
- Status changed from needs_review to positive_review
Yoooooo !
It does, because Hasse diagram is
DiGraph
. Just like for exampleP._hasse_diagram.is_connected()
is available.
Oops right sorry :-D
In this case, however, you should also see a mention of 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.
Works for me, thanks !
Nathann
comment:20 Changed 6 years ago by
No more comments for now because of #17498. Maybe I'll get back to this after it has been applied.
comment:21 Changed 6 years ago by
- Branch changed from public/11284 to 329de0c33099d582cede2d26c307cf3da4d7ae16
- Resolution set to fixed
- Status changed from positive_review to closed
Relating to this: Where is default pink color defined? For example
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 likeor
Where P would be suitable poset.