Change default behaviour of Poset to facade = True

[nathanncohen]

As mentionned in the following discussions :

• http://groups.google.com/forum/#!msg/sage-devel/qQzzZEh-JRo/WxU_qyxH2igJ
• http://groups.google.com/d/topic/sage-combinat-devel/bfWbG54sedM/discussion (sage-combinat fork of the discussion)
• https://groups.google.com/d/topic/sage-devel/xQxQLkm-RUU/discussion

This patch changes the default behaviour of the Poset constructor to facade = True, so that the elements that you define the Poset with are the elements it contains.

Two comments about this ticket :

• While the documentation of the poset/ files say that facade is a boolean, it is not. Its former default value is actually None, and the code itself tests several times if facade is None. No idea why. All I know is that I first tried to make it really boolean, and to replace those None tests by if not facade, but I was not able to make all tests pass without any idea why. I added a warning about this in the file's header, and the doctests based on the former versions of Posets are now defined with a facade = None flag. Even though it should be a boolean. Well, if this wasn't a problem before, no reason why it should become one now. This clearly has to be fixed too.
• There is a Poset.linear_extensions method whose default behaviour is also facade = None. Despite taking an optional flag, this method DOES NOT WORK as it is clearly indicated in its docstring. One can easily wonder what the hell this function is doing in Sage's code. Hence this function's behaviour is left untouched. It will use facade=None by default until it is rewritten correctly, or removed.

Nathann

Apply:

• attachment: trac_13747.patch
• attachment: trac_13747-doctests.patch
• attachment: trac_13747_reviewer.patch
• attachment: trac_13747-rebase-11763.patch
• attachment: trac_13747-rebase-12930.patch

Depends on #11763
Depends on #12930

CC: @nthiery @hivert @dimpase

Component: combinatorics

Author: Nathann Cohen

Reviewer: Christian Kuper

Merged: sage-5.6.beta2

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

[nthiery]

comment:2

facade=None just means that the facade option is not specified at this stage. The default value will be decided at a lower level, or from the context.

Please edit the description to something more constructive (that you don't know the rationale for something doesn't mean that there is no rationale in the first place and that you can insult at will those who wrote it). Once done, you can erase this comment too.

[nathanncohen]

comment:3

Replying to @nthiery:

Please edit the description to something more constructive (that you don't know the rationale for something doesn't mean that there is no rationale in the first place and that you can insult at will those who wrote it). Once done, you can erase this comment too.

Hey Nicolas, when you DO WRITE in the documentation that a thing is binary, make it binary. Else, you HAVE TO explain what None means. What you have in your head when you look at code stays in your head. What am I guilty of in this case ? Not guessing what you intended ? There are PROBLEMS with the combinat documentation, and each time a stupid guy like me is wasting his week-end and going to sleep at 4am in order to fix your bugs and write your documentation (and bear in mind that it is precisely what is happening right now), this mess is improving a bit. But there is no reason why I should be expected to know what should have been written in the file all along.

Nathann

[nathanncohen]

comment:5

Still editing the doc. I am about to try to replace some facade = None by facade = False which I tried earlier and failed to do, but I was wondering if you thought having facade = None in this file still made sense ? I mean, having undefined parameters like that is something you do when it is the default, but it feels weird to see a user explicitely define facade as None. Tell me what you think ! I'll continue editing the file, add some doc, stuff like that.

Nathann

[nathanncohen]

comment:6

Patch updated ! With a brand new doc for posets. I fixed the doc (the functions that have an optional facade argument should at the very least say which values it can take, especially if the examples use it). I removed the warning at the top because you know what you are doing, ad in the end all went well with facade = False in the examples. In my former attempts I had modified stuff in the constructors (replacing facade is None by not facade) when I thought it would make no difference, but that was a mistake).

I hope that this class is better now than it was before.

Cheers !

Nathann

[nathanncohen]

comment:7

By the way, I just took another look at the documentation of Posets, and I wondered whether something may be added in the file's header to say that facade = None is not the default anymore... What do you think ? I removed the warning that I added before, so there's an empty space for a warning to be filled :-P

Nathann

[sagetrac-christiankuper]

comment:9

Hello Nathann,

a few remarks from my side:

• Yes, the doc is better :-)

• I still don't feel very comfortable with the following facade doc: " the facade variable will be set to True or False by a lower-level call, or depending on the context." Depending on what context? I believe the doc should clearly state what influence a certain parameter has.

• In the examples you sometimes changed the former default facade value (i.e. facade = None) to facade = False, in other examples you left the coding as it was so the examples work with the new defualt value facade = True. (See 1st and 2nd example of Posets(...)). Not sure whether this was done on purpose of forgotten

• I think there is one "`" to much in the Output doc of the constructor (class FinitePoset)

Cheers

Christian

[nathanncohen]

comment:10

Hellooooooo Christian !!!

a few remarks from my side:

• Yes, the doc is better :-)

Cool :-D

• I still don't feel very comfortable with the following facade doc: " the facade variable will be set to True or False by a lower-level call, or depending on the context." Depending on what context? I believe the doc should clearly state what influence a certain parameter has.

Ahem. I believe so, too... But from the comments on this ticket you can see that I just said there what Nicolas gave me to chew. What this ticket initially contained is a warning like "There are three values for a boolean instead of three, the third is not well defined and should not be trusted -- it should be removed eventually or documented". But well, Nicolasgave me that explanation and honestly I do not mind much leaving it like that for the moment -- it is still much better than before and it will take manyyyyyyyyyyyyyyyy patches before this file is cleaned. Besides, that's not a "local" problem. Facades are used in many places and I do not expect their documentation to be very different.

• In the examples you sometimes changed the former default facade value (i.e. facade = None) to facade = False, in other examples you left the coding as it was so the examples work with the new defualt value facade = True. (See 1st and 2nd example of Posets(...)). Not sure whether this

was done on purpose of forgotten

Oh. Yeah, easy. Many explanations usually : as the new default is facade = True, I felt like this should be heavily tested in the docstrings. So when a docstring did not fail after that change, I left it like that. If it worked by changing it to facade = False, I was happy. If that did not work either I changed it to facade = None which was the former default.

Not that on some other occasions the documentation says "let us us facade = True in the following example", and in those cases (because is was explicitely said) I let facade = True stay, even though it is now the default too.

• I think there is one "`" to much in the Output doc of the constructor (class FinitePoset)

Oh ? Noooooooooo ! Actually, what I want to write is "whether the Poset's elements", but I wrapped Poset inside of a :meth: tag... But it appears in the documentation as "whether the Poset's elements" as it should, and Sphinx compiles without warning :-)

Nathann

[nathanncohen]

comment:11

(just had a chat with Nicolas Thiery, who told me that None should behave like the default input, and so that the default input should stay None, and so that the meaning of None should be facade = True when there is a doubt.)

Patch updated. Still waiting for a review !

Nathann

[sagetrac-christiankuper]

comment:12

Hello Nathann,

I only had the time to run a few tests up to now:

• Applying the patch: No issues
• "Short" doctest: All passed
• "Long" doctest: Produces a number of failures. At least at a first glance some of them look related to the module you modified.

These are the failures:

• sage -t -long devel/sage-test/sage/categories/finite_lattice_posets.py # 1 doctests failed
• sage -t -long devel/sage-test/sage/categories/posets.py # 1 doctests failed
• sage -t -long devel/sage-test/sage/categories/finite_posets.py # 9 doctests failed
• sage -t -long devel/sage-test/sage/categories/coxeter_groups.py # 1 doctests failed
• sage -t -long devel/sage-test/sage/schemes/generic/algebraic_scheme.py # 69 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/homset.py # 32 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/divisor_class.pyx # 50 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/morphism.py # 40 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/library.py # 78 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/chow_group.py # 195 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/variety.py # 199 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/divisor.py # 260 doctests failed
• sage -t -long devel/sage-test/sage/schemes/toric/fano_variety.py # 9 doctests failed
• sage -t -long devel/sage-test/sage/geometry/fan_isomorphism.py # 14 doctests failed
• sage -t -long devel/sage-test/sage/geometry/fan_morphism.py # 136 doctests failed
• sage -t -long devel/sage-test/sage/geometry/toric_plotter.py # 17 doctests failed
• sage -t -long devel/sage-test/sage/geometry/fan.py # 130 doctests failed
• sage -t -long devel/sage-test/sage/geometry/triangulation/element.py # 1 doctests failed
• sage -t -long devel/sage-test/sage/geometry/polyhedron/representation.py # 6 doctests failed
• sage -t -long devel/sage-test/sage/geometry/polyhedron/base.py # 26 doctests failed
• sage -t -long devel/sage-test/sage/geometry/cone.py # 93 doctests fail

Christian

[sagetrac-christiankuper]

Reviewer: Christian Kuper

[nathanncohen]

comment:13

>_<

Thank you so much for telling me... God. I did not think tht it would be that bad :-/

You will get it today.

Thanks again.

Nathann

[nathanncohen]

comment:14

Looks like I finally found my way through this chaos ! :-D

The only problems actually came from a couple of files, and everything was solved once this was fixed. I still have a problem with a couple of doctests in geometry/cone.py, and ask for some help on sage-devel.

https://groups.google.com/d/topic/sage-devel/Bbs_65wxKY8/discussion

If it turns out to be harmless, I will just change the doctests and this patch will be ready for a review :-)

Nathann

[nathanncohen]

comment:15

Apply trac_13747.patch, trac_13747-doctests.patch

[nathanncohen]

comment:16

Solved ! And easily ! Thanks to Volker ! :-)

Nathann

[sagetrac-christiankuper]

comment:23

Ok, I feel comfortable with the patch. As there seem to be no more open issues I will set it to positive review.

[nathanncohen]

comment:24

Thaaaaaaaaanks ! :-)

Nathann

[nthiery]

comment:25

Thanks for the patch and the review. Now the next step is to send a big fat warning on sage-combinat-devel and sage-devel about this backward incompatible change!

[jdemeyer]

Dependencies: #11763

[jdemeyer]

comment:26

This patch conflicts with #11763. I propose that this patch gets rebased to #11763.

[dimpase]

comment:27

Replying to @jdemeyer:

This patch conflicts with #11763. I propose that this patch gets rebased to #11763.

yeah, my bad (as the reviewer of #11763). I should have proposed it as a dependence here.

[nathanncohen]

Attachment: trac_13747.patch.gz

[nathanncohen]

comment:28

Attachment: trac_13747-doctests.patch.gz

Patches updates, I'm running tests right now.

Nathann

[nathanncohen]

comment:29

Apply trac_13747.patch, trac_13747-doctests.patch, trac_13747_reviewer.patch

[nathanncohen]

comment:30

Because of new occurrences of ".element" in 11763, this ticket needs another patch to pass doctests... It's just about removing those occurrences, nothing smart involved.

Apply trac_13747.patch, trac_13747-doctests.patch, trac_13747_reviewer.patch, trac_13747-rebase-11763.patch

[jdemeyer]

comment:31

Attachment: trac_13747-rebase-11763.patch.gz

sage -t  -force_lib devel/sage/sage/combinat/alternating_sign_matrix.py
**********************************************************************
File "/release/merger/sage-5.6.beta2/devel/sage-main/sage/combinat/alternating_sign_matrix.py", line 113:
    sage: L.category()
Expected:
    Category of finite lattice posets
Got:
    Category of facade finite lattice posets
**********************************************************************

[nathanncohen]

comment:32

Oh. This is because of #12930 that got merged in the meantime.

Nathann

[nathanncohen]

Changed dependencies from #11763 to #11763, #12930

[nathanncohen]

comment:33

Updated ! The most sensible was to change the doctest's output.

Nathann

[nathanncohen]

Attachment: trac_13747-rebase-12930.patch.gz

[jdemeyer]

Merged: sage-5.6.beta2