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 )
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 actuallyNone
, and the code itself tests several timesif facade is None
. No idea why. All I know is that I first tried to make it really boolean, and to replace thoseNone
tests byif 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 afacade = 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 alsofacade = 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 usefacade=None
by default until it is rewritten correctly, or removed.
Nathann
Apply:
Attachments (5)
Change History (40)
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 7 years ago by
comment:3 in reply to: ↑ 2 Changed 7 years ago by
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
- Status changed from needs_review to needs_work
comment:5 Changed 7 years ago by
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
- 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
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
- Cc dimpase added
comment:9 follow-up: ↓ 10 Changed 7 years ago by
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 toTrue
orFalse
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
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 toTrue
orFalse
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
(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
- 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
>_<
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
- 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
Apply trac_13747.patch, trac_13747-doctests.patch
comment:16 Changed 7 years ago by
- Status changed from needs_info to needs_review
Solved ! And easily ! Thanks to Volker ! :-)
Nathann
comment:17 Changed 7 years ago by
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: ↓ 19 Changed 7 years ago by
- 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
- 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 = fSeriously???
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
comment:20 Changed 7 years ago by
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
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
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
- 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
Thaaaaaaaaanks ! :-)
Nathann
comment:25 Changed 7 years ago by
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: ↓ 27 Changed 7 years ago by
- Dependencies set to #11763
- Status changed from positive_review to needs_work
comment:27 in reply to: ↑ 26 Changed 7 years ago by
Changed 7 years ago by
Changed 7 years ago by
comment:28 Changed 7 years ago by
Patches updates, I'm running tests right now.
Nathann
comment:29 Changed 7 years ago by
Apply trac_13747.patch, trac_13747-doctests.patch, trac_13747_reviewer.patch
comment:30 Changed 7 years ago by
- 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
comment:31 Changed 7 years ago by
- 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
Oh. This is because of #12930 that got merged in the meantime.
Nathann
comment:33 Changed 7 years ago by
- 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
comment:34 Changed 7 years ago by
- Description modified (diff)
comment:35 Changed 7 years ago by
- Merged in set to sage-5.6.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
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.