PentagonPoset and DiamondPoset, default argument for facade

[jm58660]

This triggers error:

P1=Posets.ChainPoset(3)
P2=Posets.PentagonPoset().relabel(lambda x: x+10)
P=Poset( (P1.list()+P2.list(), P1.cover_relations()+P2.cover_relations()) )
P.show()

Error message is ValueError: element (=2) not in poset. Reason was default value for argument facade. This is corrected in this ticket.

Same bug was in Posets.DiamondPoset(). Also Posets.DiamondPoset(2) returns empty poset, DiamondPoset(1) gives error message. This is now changed in for n <= 2 they raise exception.

CC: @nathanncohen

Component: combinatorics

Author: Jori Mäntysalo

Branch/Commit: 044a0a0

Reviewer: Nathann Cohen

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

[jm58660]

comment:1

Is this related to #14019? You don't get same error message if you say P.cover_relations() and then manually make same poset. So there is something with relabel() happening.

[nathanncohen]

comment:2

Okay, I'll add a message in #14019, they have been wasting my time for long enough.

[jm58660]

comment:3

On discussion at #14019 tscrim wasn't sure if this relates to same relabeling thing or not. I move this to wontfix-milestone, but will not marked as positive review yet.

[jm58660]

comment:4

More about this. Problem is that type(Posets.PentagonPoset().list()[0]) is sage.combinat.posets.elements.FiniteLatticePoset_with_category.element_class, whereas for example type(Posets.RandomPoset(1,0).list()[0]) is sage.rings.integer.Integer. Problem can be seen for example by

P1=Posets.PentagonPoset()
P2=Poset({'a':['b','c']})
P=Poset( (P1.list()+P2.list(), []) )
P.show()

[jm58660]

comment:6

More fun:

x=Poset(Posets.PentagonPoset().hasse_diagram()).list()[1]
print x
print x+1

outputs

2
4

[nathanncohen]

comment:7

I HATE THIS CODE. You cannot for one second even trust that it does what you think it should do.

Okay, that's because there is in Poset(....) a keyword named "facade" (not how clearly it indicates what it does) that changes the type of the things inside of the poset.

sage: d = DiGraph({0:[1]})
sage: map(type,Poset(d))
[sage.rings.integer.Integer, sage.rings.integer.Integer]
sage: map(type,Poset(d,facade=False))
[sage.combinat.posets.elements.FinitePoset_with_category.element_class,
 sage.combinat.posets.elements.FinitePoset_with_category.element_class]

I had to fight a looooong time ago to obtain that facade becomes True by defaut in Posets. And now it is, but for some stupid reason the constructor of PentagonalPoset enforces facade=False. So the code should only be changed to follow the same default as the Poset constructor. Or even to remove this keyword in PentagonalPoset, or replace it with **kwds... Anything -_-

By the way: the real explanation is that the + has been changed to mean "join" or "meet". It is not had to find out: take your 'x' as you defined it and type 'x.add'.

Nathann

[jm58660]

Branch: u/jmantysalo/strange_bug_in_poset_show__

[jm58660]

Commit: 75004ee

[jm58660]

comment:9

I also removed part of documentation; it seems to better suited for is_distributive().

---

New commits:

75004ee

Set default value of facade to True. Shortened documentation.

[jm58660]

Author: Jori Mäntysalo

[nathanncohen]

comment:10

The same thing happens with DiamontPoset just afterwards.

Nathann

[sagetrac-git]

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

62d990c

Combined two tickets.

[sagetrac-git]

Changed commit from 75004ee to 62d990c

[nathanncohen]

Reviewer: Nathann Cohen

[nathanncohen]

comment:14

Helloooooooooooooo !

Seems good, but why did you remove that doc ?

Also, could you mention this n>=3 in the doc of DiamondPoset ?

Thanks,

Nathann

[sagetrac-git]

Changed commit from 62d990c to d19213c

[sagetrac-git]

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

d19213c

Better documentation.

[jm58660]

comment:16

Replying to @nathanncohen:

Seems good, but why did you remove that doc ?

I think that it was irrelevant in this context. Also note about DiamondPoset seems to be at wrong place inside docstring of PentagonPoset.

Also, could you mention this n>=3 in the doc of DiamondPoset ?

Done. Should have noticed this myself.

[nathanncohen]

comment:17

Hello !

I think that it was irrelevant in this context. Also note about DiamondPoset seems to be at wrong place inside docstring of PentagonPoset.

Why do you think that it is irrelevant here ? It is a docstring about an object, so why don't we show its nice properties ? This sage software is all about checking on computers what we can prove on paper anyway :-)

Nathann

[sagetrac-git]

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

afbad02

Added an example to PentagonPoset().

[sagetrac-git]

Changed commit from d19213c to afbad02

[jm58660]

comment:19

OK, I added an example. Not the same, thought.

[nathanncohen]

comment:21

Hello !

I added a small commit on top of yours. What it does :

1. Remove the " in the first line of doc of the PentagonPoset constructor.

2. Improve the doc of the facade keyword

3. Bring the former doctest back. The point is that doctests are not only useful to the users, but also to test our own code. I can't tell you how many bugs are avoided just because there is always a doctest somewhere that tests the corner case you did not think about. If we ever change the data structure of Posets, do anything in the graphs backends, those doctests are the only way we have to make sure everything is holding together.

4. Changing the default value of facade was breaking doctests in another file, so I updated them. In order to check the doctests use sage -b && sage -tp <num_of_cpu> -l file_or_directory

I will also create a ticket to ask Nicolas to write some documentation about Facade, because right now there is none to find.

Set this ticket to positive_review if you agree with the changes.

Nathann

[nathanncohen]

Changed branch from u/jmantysalo/strange_bug_in_poset_show__ to public/17051

[nathanncohen]

Changed commit from afbad02 to 6cf7db8

[nathanncohen]

New commits:

6cf7db8

trac #17051: Review

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

044a0a0

trac #17051: Review

[sagetrac-git]

Changed commit from 6cf7db8 to 044a0a0

[nathanncohen]

comment:24

I had forgotten a detail: it is better to make the default value of facade be equal to None, as it means "use the same default as in Poset(). We should not have to repeat the default value everywhere. That's why we had the problem in the first place.

Nathann

[nathanncohen]

comment:25

(just created #17073 about the documentation of Facade)

[jm58660]

comment:26

Seems reasonable --> positive_review.

[vbraun]

Changed branch from public/17051 to 044a0a0