Opened 6 years ago

Closed 6 years ago

#17477 closed enhancement (wontfix)

"Internal" documentation of posets

Reported by: jmantysalo Owned by:
Priority: major Milestone: sage-duplicate/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 ncohen)

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

Change History (24)

comment:1 Changed 6 years ago by jmantysalo

  • Branch set to u/jmantysalo/_internal__documentation_of_posets

comment:2 Changed 6 years ago by git

  • Commit set to e32150e67d4a5e1935754d698b35969b64cc80b6

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

e32150eCorrections.

comment:3 Changed 6 years ago by jmantysalo

  • Milestone changed from sage-wishlist to sage-6.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 follow-up: Changed 6 years ago by ncohen

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

comment:5 in reply to: ↑ 4 Changed 6 years ago by jmantysalo

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

comment:6 follow-up: Changed 6 years ago by ncohen

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

comment:7 Changed 6 years ago by ncohen

  • Branch changed from u/jmantysalo/_internal__documentation_of_posets to public/17477
  • Commit changed from e32150e67d4a5e1935754d698b35969b64cc80b6 to 068297887ef00cfbf950efa2e18f919c021e6e4e

New commits:

0682978trac #17477: Reviewer's commit

comment:8 Changed 6 years ago by git

  • Commit changed from 068297887ef00cfbf950efa2e18f919c021e6e4e to 8ecc92457eef2cc2264bc7bcb2ef3f03d57add75

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

8ecc924Removed incorrect information about node positioning for plot.

comment:9 in reply to: ↑ 6 Changed 6 years ago by jmantysalo

  • Reviewers set to Nathann Cohen

Replying to ncohen:

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.

comment:10 follow-up: Changed 6 years ago by 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...

Anyway: it this ticket ready ? If you have nothing to add I guess it can go.

Nathann

comment:11 Changed 6 years ago by ncohen

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

comment:12 in reply to: ↑ 10 Changed 6 years ago by jmantysalo

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

comment:13 Changed 6 years ago by ncohen

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 211 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 (211)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

comment:14 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:15 Changed 6 years ago by vbraun

author name missing

comment:16 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:17 Changed 6 years ago by jmantysalo

  • Authors set to Jori Mäntysalo
  • Status changed from needs_work to needs_review

comment:18 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:19 Changed 6 years ago by jmantysalo

  • 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 ncohen

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 jmantysalo

I opened a discussion on sage-devel about this.

comment:22 Changed 6 years ago by ncohen

  • Description modified (diff)

comment:23 Changed 6 years ago by jmantysalo

  • Milestone changed from sage-6.5 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to positive_review

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.

comment:24 Changed 6 years ago by vbraun

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.