Opened 6 years ago
Closed 6 years ago
#17477 closed enhancement (wontfix)
"Internal" documentation of posets
Reported by:  jmantysalo  Owned by:  

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  combinatorics  Keywords:  poset 
Cc:  ncohen  Merged in:  
Authors:  Jori Mäntysalo  Reviewers:  Nathann Cohen 
Report Upstream:  N/A  Work issues:  
Branch:  public/17477 (Commits)  Commit:  8ecc92457eef2cc2264bc7bcb2ef3f03d57add75 
Dependencies:  Stopgaps: 
Description (last modified by )
Poset class should contain some lines of documentation meant mostly for developer. Basically "A poset is a digraph with integers as elements and a list..."
Discussion at https://groups.google.com/d/topic/sagedevel/ZXkfB5dSchs/discussion
Change History (24)
comment:1 Changed 6 years ago by
 Branch set to u/jmantysalo/_internal__documentation_of_posets
comment:2 Changed 6 years ago by
 Commit set to e32150e67d4a5e1935754d698b35969b64cc80b6
comment:3 Changed 6 years ago by
 Milestone changed from sagewishlist to sage6.5
 Status changed from new to needs_review
As there is no status "needs_comments" I put this to needs_review. Comments are welcome.
(My native language belongs to Uralic languages... Hence maybe not best one to write documentation in English.)
comment:4 followup: ↓ 5 Changed 6 years ago by
Hello !
I fixed a couple of things and added links toward the methods/clases you cite (branch at public/17477)
Also, I am not sure that the Poset
contain as you claim the position of the vertices. I believe that it is computed onthefly when FinitePoset.plot
is called.
I set this ticket back to needs_info
because of this last comment.
Nathann
comment:5 in reply to: ↑ 4 Changed 6 years ago by
Replying to ncohen:
I fixed a couple of things and added links toward the methods/clases you cite (branch at public/17477)
Looks better.
Also, I am not sure that the
Poset
contain as you claim the position of the vertices. I believe that it is computed onthefly whenFinitePoset.plot
is called.
True. Actually docs of Hasse diagram says that plot()
takes argument label_font_size
. It does, but do not do anything with it  i.e. P._hasse_diagram.plot(label_font_size='catsaysmeow').show()
equals to P._hasse_diagram.plot(label_font_size=100).show()
.
Is this easier if you just remove paragraph "The Internal DiGraph? of the poset also contains  " from public/17477 and make it to branch for this repo? Maybe also we can add to last paragraph something like "Hence for example P.gt(x, y)
if faster after P.lequal_matrix()
."
comment:6 followup: ↓ 9 Changed 6 years ago by
Hello !
True. Actually docs of Hasse diagram says that
plot()
takes argumentlabel_font_size
. It does, but do not do anything with it  i.e.P._hasse_diagram.plot(label_font_size='catsaysmeow').show()
equals toP._hasse_diagram.plot(label_font_size=100).show()
.
Oh. I see. Could you do something for that ? If you cannot, please tell me and I will. It must not be forgotten again.
Is this easier if you just remove paragraph "The Internal DiGraph? of the poset also contains  " from public/17477 and make it to branch for this repo? Maybe also we can add to last paragraph something like "Hence for example
P.gt(x, y)
if faster afterP.lequal_matrix()
."
Well, can you add a commit on the branch for that ? I change the ticket's branch so that we can see what is happening.
Nathann
comment:7 Changed 6 years ago by
 Branch changed from u/jmantysalo/_internal__documentation_of_posets to public/17477
 Commit changed from e32150e67d4a5e1935754d698b35969b64cc80b6 to 068297887ef00cfbf950efa2e18f919c021e6e4e
New commits:
0682978  trac #17477: Reviewer's commit

comment:8 Changed 6 years ago by
 Commit changed from 068297887ef00cfbf950efa2e18f919c021e6e4e to 8ecc92457eef2cc2264bc7bcb2ef3f03d57add75
Branch pushed to git repo; I updated commit sha1. New commits:
8ecc924  Removed incorrect information about node positioning for plot.

comment:9 in reply to: ↑ 6 Changed 6 years ago by
 Reviewers set to Nathann Cohen
Replying to ncohen:
True. Actually docs of Hasse diagram says that
plot()
takes argumentlabel_font_size
. It does, but do not do anything with it
Oh. I see. Could you do something for that ? If you cannot, please tell me and I will. It must not be forgotten again.
I think that this is not about one option. Whole plot()
should be checked: to make it possible to have noninjective relabeling and to check options. So, feel free to correct it. :=)
Well, can you add a commit on the branch for that ? I change the ticket's branch so that we can see what is happening.
Done this.
comment:10 followup: ↓ 12 Changed 6 years ago by
Hmmmm.. Now that I think of it, the function #17408 actually computes the lequal_matrix
when computing the transitive reduction, but we throw it away...
Anyway: it this ticket ready ? If you have nothing to add I guess it can go.
Nathann
comment:11 Changed 6 years ago by
I think that this is not about one option. Whole
plot()
should be checked: to make it possible to have noninjective relabeling and to check options.
The problem comes from HasseDiagram.plot
. This argument, and two others, are totally ignored. Also, it seems that this HasseDiagram.plot
function is never used: Poset.plot
calls GenericGraph.plot
directly.
Nathann
comment:12 in reply to: ↑ 10 Changed 6 years ago by
Replying to ncohen:
Hmmmm.. Now that I think of it, the function #17408 actually computes the
lequal_matrix
when computing the transitive reduction, but we throw it away...
Aha. I think that Sage could have some kind of systemlevel setting for memorycpu tradeoff. Sometimes saving, for example, lematrix, may not be right thing to do.
Anyway: it this ticket ready ? If you have nothing to add I guess it can go.
Yes, I think this is ready. Or at least better than no documentation at all.
comment:13 Changed 6 years ago by
Aha. I think that Sage could have some kind of systemlevel setting for memorycpu tradeoff. Sometimes saving, for example, lematrix, may not be right thing to do.
Ahahaha. No, I swear that in our current implementation it is *always* the right thing to do. The term 'dense matrix' may be scary, but a matrix of bits is infinitely more compact than a list of adjacences stored with pointers and stuff. Infinitely more compact that all Python objects we store for nothing.
Look: the BooleanLattice
poset on 2^{11 elements seems to take 320Mb in memory (no idea why it is so expensive):
}
sage: m1 = get_memory_usage() sage: p = posets.BooleanLattice(11) sage: round(get_memory_usage()m1) 321.0
How much does it cost to store the dense binary matrix ? Simple, you need (2^{11)}2 bits. That means ... 4 Mb = 500kB.
And we (still) live in a world where all posets you generate are stored forever uselessly, where you have 64bits pointers everywhere and stuff. Really, those 4Mb are far from accounting for any of our memory problems :P
Yes, I think this is ready. Or at least better than no documentation at all.
Okay, positive review then !
Nathann
comment:14 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:15 Changed 6 years ago by
author name missing
comment:16 Changed 6 years ago by
 Status changed from positive_review to needs_work
comment:17 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:18 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:19 Changed 6 years ago by
 Status changed from positive_review to needs_work
Duh. We already have this kind of documentation on __init__
of poset.
comment:20 Changed 6 years ago by
Oh.. I see _
Then I guess you should move what is useful in there to the documentation you added in the module, and delete what is left. Doc is no good if nobody knows it is there, and you cannot easily get the doc content of __init__
in Sage. Plus it does not appear in the html.
Nathann
comment:21 Changed 6 years ago by
I opened a discussion on sagedevel about this.
comment:22 Changed 6 years ago by
 Description modified (diff)
comment:23 Changed 6 years ago by
 Milestone changed from sage6.5 to sageduplicate/invalid/wontfix
 Status changed from needs_work to positive_review
There was no clear consensus on sagedevel. Hence I mark this as positive_review and wont_fix.
Maybe I'll get back to this when going throught whole documentation of posets.
comment:24 Changed 6 years ago by
 Resolution set to wontfix
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Corrections.