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 )
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
- 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 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: ↓ 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 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
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 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='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: ↓ 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='cat-says-meow').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 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: ↓ 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 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
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
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
- 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 sage-devel about this.
comment:22 Changed 6 years ago by
- Description modified (diff)
comment:23 Changed 6 years ago by
- 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
- Resolution set to wontfix
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Corrections.