"Internal" documentation of posets

[jm58660]

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/sage-devel/ZXkfB5dSchs/discussion

CC: @nathanncohen

Component: combinatorics

Keywords: poset

Author: Jori Mäntysalo

Branch/Commit: public/17477 @ 8ecc924

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/17477

[jm58660]

Branch: u/jmantysalo/_internal__documentation_of_posets

[sagetrac-git]

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

e32150e

Corrections.

[sagetrac-git]

Commit: e32150e

[jm58660]

comment:3

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.)

[nathanncohen]

comment:4

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 on-the-fly when FinitePoset.plot is called.

I set this ticket back to needs_info because of this last comment.

Nathann

[jm58660]

comment:5

Replying to @nathanncohen:

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 on-the-fly when FinitePoset.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='cat-says-meow').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()."

[nathanncohen]

comment:6

Hello !

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='cat-says-meow').show() equals to P._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 after P.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

[nathanncohen]

New commits:

0682978

trac #17477: Reviewer's commit

[nathanncohen]

Changed commit from e32150e to 0682978

[nathanncohen]

Changed branch from u/jmantysalo/_internal__documentation_of_posets to public/17477

[sagetrac-git]

Changed commit from 0682978 to 8ecc924

[sagetrac-git]

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

8ecc924

Removed incorrect information about node positioning for plot.

[jm58660]

Reviewer: Nathann Cohen

[jm58660]

comment:9

Replying to @nathanncohen:

True. Actually docs of Hasse diagram says that plot() takes argument label_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 non-injective 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.

[nathanncohen]

comment:10

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

[nathanncohen]

comment:11

I think that this is not about one option. Whole plot() should be checked: to make it possible to have non-injective 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

[jm58660]

comment:12

Replying to @nathanncohen:

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 system-level setting for memory-cpu -tradeoff. Sometimes saving, for example, le-matrix, 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.

[nathanncohen]

comment:13

Aha. I think that Sage could have some kind of system-level setting for memory-cpu -tradeoff. Sometimes saving, for example, le-matrix, 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¹¹⁾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 64-bits 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

[vbraun]

comment:15

author name missing

[jm58660]

Author: Jori Mäntysalo

[jm58660]

comment:19

Duh. We already have this kind of documentation on __init__ of poset.

[nathanncohen]

comment:20

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

[jm58660]

comment:21

I opened a discussion on sage-devel about this.

[jm58660]

comment:23

There was no clear consensus on sage-devel. Hence I mark this as positive_review and wont_fix.

Maybe I'll get back to this when going throught whole documentation of posets.