Opened 7 years ago

Closed 7 years ago

#13747 closed defect (fixed)

Change default behaviour of Poset to facade = True

Reported by: ncohen Owned by: sage-combinat
Priority: major Milestone: sage-5.6
Component: combinatorics Keywords:
Cc: nthiery, fhivert, dimpase Merged in: sage-5.6.beta2
Authors: Nathann Cohen Reviewers: Christian Kuper
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11763, #12930 Stopgaps:

Description (last modified by ncohen)

As mentionned in the following discussions :

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:

Attachments (5)

trac_13747_reviewer.patch (3.2 KB) - added by novoselt 7 years ago.
trac_13747.patch (33.0 KB) - added by ncohen 7 years ago.
trac_13747-doctests.patch (15.1 KB) - added by ncohen 7 years ago.
trac_13747-rebase-11763.patch (3.2 KB) - added by ncohen 7 years ago.
trac_13747-rebase-12930.patch (713 bytes) - added by ncohen 7 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 7 years ago by ncohen

  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by nthiery

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.

comment:3 in reply to: ↑ 2 Changed 7 years ago by ncohen

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

comment:4 Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work

comment:5 Changed 7 years ago by ncohen

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

comment:6 Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_review

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

comment:7 Changed 7 years ago by ncohen

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

comment:8 Changed 7 years ago by ncohen

  • Cc dimpase added

comment:9 follow-up: Changed 7 years ago by christiankuper

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

comment:10 in reply to: ↑ 9 Changed 7 years ago by ncohen

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

comment:11 Changed 7 years ago by ncohen

(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

comment:12 Changed 7 years ago by christiankuper

  • Reviewers set to Christian Kuper
  • Status changed from needs_review to needs_work

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

comment:13 Changed 7 years ago by ncohen

>_<

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

comment:14 Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_info

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

comment:15 Changed 7 years ago by ncohen

Apply trac_13747.patch, trac_13747-doctests.patch

comment:16 Changed 7 years ago by ncohen

  • Status changed from needs_info to needs_review

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

Nathann

comment:17 Changed 7 years ago by christiankuper

Hello Nathann,

as far as I can judge there are no more issues with this patch:

  • All test passed successfully
  • The doc look ok
  • The methods are behaving as promised in the doc (default behavior with facade = True etc)

BTW, the explanation for the facade = True / False / None bevavior is much clearer than in your first version ;-)

Just be clear about the extension of my review: I focused ONLY on the changes you made. IMHO there is still room for improvment in this module, as you stated earlier. ;-)

Good work!

Christian

comment:18 follow-up: Changed 7 years ago by novoselt

  • Status changed from needs_review to needs_work

The changes in sage/geometry/fan.py do not look OK to me:

f = f 

Seriously???

'agree'

instead of

"agree"

just a change for the sake of change?

(e for e in level)

Not much better than the first one and it is repeated in sage/geometry/cone.py. If fixing doctests was done by some script/RE, it would be nice to read the result, especially if the code got changed, not just doctests as the patch name suggests.

comment:19 in reply to: ↑ 18 Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_review

Hellooooooooooooooooo !!!

The changes in sage/geometry/fan.py do not look OK to me:

And indeedthey aren't :-P

f = f 

Seriously???

You're not in the right frame of mind to appreciate such a change. Here is a quote from the Wikipedia Page on monochrome paintings (it actually talks about things like that : http://en.wikipedia.org/wiki/File:Black_square.jpg)


Suprematism and Constructivism

Monochrome painting as it is usually understood today began in Moscow, with Suprematist Composition: White on White [5] of 1918 by Suprematist artist Kazimir Malevich. This was a variation on or sequel to his 1915 work “Black Square on a White Field”, a very important work in its own right to 20th century geometric abstraction. In 1921, Constructivist artist Alexandr Rodchenko exhibited three paintings together, each a monochrome of one of the three primary colours. He intended this work to represent The Death of Painting.[6]

While Rodchenko intended his monochrome to be a dismantling of the typical assumptions of painting, Malevich saw his work as a concentration on them, a kind of meditation on art’s essence (“pure feeling”).

These two approaches articulated very early on in its history this kind of work’s almost paradoxical dynamic: that one can read a monochrome either as a flat surface (material entity or “painting as object”) which represents nothing but itself, and therefore representing an ending in the evolution of illusionism in painting (i.e. Rodchenko); or as a depiction of multidimensional (infinite) space, a fulfillment of illusionistic painting, representing a new evolution—a new beginning—in Western painting’s history (Malevich). Additionally, many have pointed out that it may be difficult to deduce the artist’s intentions from the painting itself, without referring to the artist’s comment.


So of course, f=f could seem a bit trivial and useless to you. You may even think that you could have done it by yourself. But the truth is that there is a depth behind that you just cannot appreciate.

I removed it from the new version of my patch, because I don't get it either.

'agree'

instead of

"agree"

just a change for the sake of change?

Totally. I love change. I first intended to replace "agree" by $#$aaaagrRRrrrrrrrrreeeeeee$#$ but hesitated when I considered the extravagant cost of the characters. This, and because emacs seems to have trouble with the " in the docstring, and for this reason believed that in the rest of the file comments were code, and code comments... Do you mind ? ^^;

(e for e in level)

Not much better than the first one and it is repeated in sage/geometry/cone.py. If fixing doctests was done by some script/RE, it would be nice to read the result, especially if the code got changed, not just doctests as the patch name suggests.

Of course here I have to make the same remark as previously. There is depth in (e for e in level), a depth that is not reached by the expression iter(level). I changed it anyway, there and at all other places where it was needed.

Sorry for all that. It's just that I changed these files sooooooooooooo many times that I was a bit too excited when it passed all doctests. Thank you for looking at it.

Have fuuuuuuuuuuuuuuuun !

Nathann

P.S. : By the way, if only to relieve you of a legitimate fear if you thought that all this was done without me looking : I indeed did most of these changes with emacs macros, but each time something was about to be changed in the code emacs asked me whether I agreed with the change (he's that kind). I only changed code when I thought the change made sense and would not change the code's meaning. As I said, I changed this code in so many ways, hoping to finally get the doctests to pass, that I just wanted to see whether the method worked. And if not, just delete the patch and try another way :-)

Changed 7 years ago by novoselt

comment:20 Changed 7 years ago by novoselt

I do mind change of quotes for no reason, just as fiddling with whitespace. I had an impression that emacs was the most configurable and smart editor ever, so certainly it can understand what is comment and what is not. Anyway, the patch on top of others removes unnecessary iterators and adjusts the flow of documentation comments that dealt with the old behaviour.

comment:21 Changed 7 years ago by ncohen

OKayyyyyyyyyyyy... Well, what do we do with this ticket now ? I've got no problem at all with your changes.. :-)

Nathann

comment:22 Changed 7 years ago by novoselt

Well, I have no more objections to the ticket and I think that new default is more intuitive, but I didn't look over the main patch, so I can't set it to positive review.

Christian - are you OK with the ticket overall?

comment:23 Changed 7 years ago by christiankuper

  • Status changed from needs_review to positive_review

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

comment:24 Changed 7 years ago by ncohen

Thaaaaaaaaanks ! :-)

Nathann

comment:25 Changed 7 years ago by nthiery

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!

comment:26 follow-up: Changed 7 years ago by jdemeyer

  • Dependencies set to #11763
  • Status changed from positive_review to needs_work

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

comment:27 in reply to: ↑ 26 Changed 7 years ago by dimpase

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.

Changed 7 years ago by ncohen

Changed 7 years ago by ncohen

comment:28 Changed 7 years ago by ncohen

Patches updates, I'm running tests right now.

Nathann

comment:29 Changed 7 years ago by ncohen

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

comment:30 Changed 7 years ago by ncohen

  • Description modified (diff)
  • Status changed from needs_work to positive_review

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

Changed 7 years ago by ncohen

comment:31 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work
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
**********************************************************************

comment:32 Changed 7 years ago by ncohen

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

Nathann

comment:33 Changed 7 years ago by ncohen

  • Dependencies changed from #11763 to #11763, #12930
  • Status changed from needs_work to positive_review

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

Nathann

Changed 7 years ago by ncohen

comment:34 Changed 7 years ago by ncohen

  • Description modified (diff)

comment:35 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.6.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.