search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.42k stars 476 forks source link

Change default behaviour of Poset to facade = True #13747

Closed 6bdad4c1-1e26-4f2f-a442-a01a2292c181 closed 11 years ago

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago

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 :

Nathann

Apply:

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 commented 11 years ago
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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

0327930e-049f-43cc-8d1e-a5d2e39c697e commented 11 years ago
comment:9

Hello Nathann,

a few remarks from my side:

Cheers

Christian

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

0327930e-049f-43cc-8d1e-a5d2e39c697e commented 11 years ago
comment:12

Hello Nathann,

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

These are the failures:

Christian

0327930e-049f-43cc-8d1e-a5d2e39c697e commented 11 years ago

Reviewer: Christian Kuper

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:15

Apply trac_13747.patch, trac_13747-doctests.patch

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:16

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

Nathann

0327930e-049f-43cc-8d1e-a5d2e39c697e commented 11 years ago
comment:17

Hello Nathann,

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

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

novoselt commented 11 years ago
comment:18

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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:19

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 :-)

novoselt commented 11 years ago

Attachment: trac_13747_reviewer.patch.gz

novoselt commented 11 years ago
comment:20

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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:21

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

Nathann

novoselt commented 11 years ago
comment:22

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?

0327930e-049f-43cc-8d1e-a5d2e39c697e commented 11 years ago
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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:24

Thaaaaaaaaanks ! :-)

Nathann

nthiery commented 11 years ago
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 commented 11 years ago

Dependencies: #11763

jdemeyer commented 11 years ago
comment:26

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

dimpase commented 11 years ago
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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago

Attachment: trac_13747.patch.gz

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:28

Attachment: trac_13747-doctests.patch.gz

Patches updates, I'm running tests right now.

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:29

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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago

Description changed:

--- 
+++ 
@@ -10,3 +10,10 @@
 * 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](https://github.com/sagemath/sage-prod/files/10656684/trac_13747.patch.gz)
+* [attachment: trac_13747-doctests.patch](https://github.com/sagemath/sage-prod/files/10656685/trac_13747-doctests.patch.gz)
+* [attachment: trac_13747_reviewer.patch](https://github.com/sagemath/sage-prod/files/10656683/trac_13747_reviewer.patch.gz)
+* [attachment: trac_13747-rebase-11763.patch](https://github.com/sagemath/sage-prod/files/10656686/trac_13747-rebase-11763.patch.gz)
+
6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
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 commented 11 years ago
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
**********************************************************************
6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:32

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

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago

Changed dependencies from #11763 to #11763, #12930

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:33

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

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago

Attachment: trac_13747-rebase-12930.patch.gz

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago

Description changed:

--- 
+++ 
@@ -16,4 +16,4 @@
 * [attachment: trac_13747-doctests.patch](https://github.com/sagemath/sage-prod/files/10656685/trac_13747-doctests.patch.gz)
 * [attachment: trac_13747_reviewer.patch](https://github.com/sagemath/sage-prod/files/10656683/trac_13747_reviewer.patch.gz)
 * [attachment: trac_13747-rebase-11763.patch](https://github.com/sagemath/sage-prod/files/10656686/trac_13747-rebase-11763.patch.gz)
-
+* [attachment: trac_13747-rebase-12930.patch](https://github.com/sagemath/sage-prod/files/10656687/trac_13747-rebase-12930.patch.gz)
jdemeyer commented 11 years ago

Merged: sage-5.6.beta2