Opened 8 years ago
Closed 7 years ago
#18223 closed enhancement (fixed)
cartesian products with orders
Reported by:  Daniel Krenn  Owned by:  

Priority:  major  Milestone:  sage6.7 
Component:  categories  Keywords:  sd67 
Cc:  Benjamin Hackl, Clemens Heuberger, Nicolas M. Thiéry  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl, Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  8f9a619 (Commits, GitHub, GitLab)  Commit:  8f9a61942e14254ec028924bccf918774fb57cf6 
Dependencies:  Stopgaps: 
Description (last modified by )
Create cartesian products which have a particular order (lexicographically or componentwise) on its elements.
This also incorporates the proposed changes of #18586 (won't be fixed; see discussion there), namely passes keyword arguments on in cartesian_product
and adds extra_category
.
Change History (47)
comment:1 Changed 8 years ago by
Branch:  → u/dkrenn/cat/cartesian 

comment:2 Changed 8 years ago by
Commit:  → 46c7a0464379815d0507e7385bb23783c4f7b5f1 

Status:  new → needs_review 
comment:3 Changed 8 years ago by
Cc:  Nicolas M. Thiéry added 

comment:4 followup: 5 Changed 8 years ago by
Branch:  u/dkrenn/cat/cartesian → public/ticket/18223 

Commit:  46c7a0464379815d0507e7385bb23783c4f7b5f1 → 1013cca1a33877e3ba52af111324be937ec19f58 
comment:5 Changed 8 years ago by
comment:6 followup: 7 Changed 8 years ago by
This is forced to me by git, even when the merge is trivial. I work like that:
1) I copy the develop branch into say branch18223
2) I pull the branch from trac, sitting on branch18223
3) git forces me to do a merge commit, often a trivial one.
comment:7 Changed 8 years ago by
Replying to chapoton:
This is forced to me by git, even when the merge is trivial. I work like that:
1) I copy the develop branch into say branch18223
2) I pull the branch from trac, sitting on branch18223
3) git forces me to do a merge commit, often a trivial one.
http://www.sagemath.org/doc/developer/manual_git.html#mergingandrebasing says at the end of the section:
[...] do nothing unless necessary: it is perfectly fine for your branch to be behind master [...]
comment:8 Changed 8 years ago by
Commit:  1013cca1a33877e3ba52af111324be937ec19f58 → b924b94bc2e1aa12021cc70b3a919532aa1e350a 

comment:9 Changed 8 years ago by
Status:  needs_review → needs_work 

Hello Daniel,
 There are tons of product orders that you can define on a cartesian product of posets. You want to write a category for each of them? It is not at all usable for people that wants to build a custom product. The aim of categories is to factorize code not to expand the amount of work. I think that you should seriously develop another strategy for providing an order to a product of posets.
 Moreover, this looks very unnatural to me:
sage: P = Poset((srange(10), lambda left, right: left <= right)) sage: Q = cartesian_product((P, P, P), ....: extra_category=Posets().CompareLexCartesianProduct())
IMHO, something like the following would be bettersage: posets.cartesian_product((P,P,P), order = my_order)
where order could also be a custom function or a string like 'lex', 'product', ... this second remark is just from the user point of view.
 If there is one product, it should be the cartesian product (or the coordinate wise product) which is the categorical one (when morphisms are the monotone functions). Though as there is a lack of definitions, I am not sure that
Hom
between two posets are the monotone functions. Would be interesting to settle that.
 Sided remark: this will not work with cython Element classes (as there is no element_class)
self.element_class.__le__ = \ lambda left, right: left.parent().le(left, right)
Vincent
comment:10 followup: 11 Changed 8 years ago by
Salut Vincent!
We had discussed this design with Daniel, and take my share of the blame. I am not quite happy with this solution. I am not quite happy with other solutions either. So that's a good occasion for a discussion!
I guess the main question is whether there will be other categories in the long run where there will be several variants for the cartesian product, and we want everything to interplay.
If not, then having a specific cartesian product for posets is probably ok.
If yes, we would want to have some syntax where we can specify options for the various structures.
sage: cartesian_product([A,B,C], poset options, xxx options, ...)
This is more or less what the proposed syntax aims for. But it has the drawbacks you mention. Possibly this would not be so bad if the category was parametrized by a "term order":
sage: cartesian_product([A,B,C], Posets().CartesianProducts(term_order="lex")
It would need to be checked whether we can indeed achieve this syntax (or a similar one) without tweaking too much the functorial construction infrastructure.
Another related question is whether a single function is sufficient to specify the order, or not: maybe we would want to provide implementations of other methods that apply for specific term orders.
Cheers,
Nicolas
comment:11 followup: 12 Changed 8 years ago by
Salut Nicolas,
Replying to nthiery:
We had discussed this design with Daniel, and take my share of the blame. I am not quite happy with this solution. I am not quite happy with other solutions either. So that's a good occasion for a discussion!
On the other hand the feature is clearly missing. So we need to find a solution.
I guess the main question is whether there will be other categories in the long run where there will be several variants for the cartesian product, and we want everything to interplay.
If not, then having a specific cartesian product for posets is probably ok.
If yes, we would want to have some syntax where we can specify options for the various structures.
sage: cartesian_product([A,B,C], poset options, xxx options, ...)
This worries me a lot since cartesian product is intended for any kind of structures... not only poset. If you start adding specific options into this machinery it will be horrible as well.
This is more or less what the proposed syntax aims for. But it has the drawbacks you mention. Possibly this would not be so bad if the category was parametrized by a "term order":
sage: cartesian_product([A,B,C], Posets().CartesianProducts(term_order="lex")
This is already nicer. Though, there is a problem with morphisms then (see my remark 3 in comment:9). The order in a cartesian product can not be parametrized if the morphisms are monotone functions. So doing the above will prevent defining morphisms in that way. I do not know whether it is what we want to do here.
Note that the following almost works
B = cartesian_product([A0, A1, A2]) my_cmp = lambda x,y: x[0] <= y[0] and x[1] <= y[1] and x[2] <= y[2] P = Poset((B, my_cmp))
I say almost, because building a Poset
is infinitely slow as always and works only for finite posets.
Vincent
comment:12 Changed 8 years ago by
Replying to vdelecroix:
Note that the following almost works
B = cartesian_product([A0, A1, A2]) my_cmp = lambda x,y: x[0] <= y[0] and x[1] <= y[1] and x[2] <= y[2] P = Poset((B, my_cmp))I say almost, because building a
Poset
is infinitely slow as always and works only for finite posets.
Just a short comment: This will not work for what I have in mind, since my A0, A1, A2 will be groups or monoids or rings, and I want B to be a group or monoid or ring, resp., as well. This is also one motivation for using the category framework to add the desired comparison behavior to existing structures.
comment:13 Changed 7 years ago by
Authors:  → Daniel Krenn 

Branch:  public/ticket/18223 → u/dkrenn/cat/cartesianproductposets 
Commit:  b924b94bc2e1aa12021cc70b3a919532aa1e350a → 9352031b310b81f83c09ecac11b1ce15163f55f9 
Dependencies:  → #18586 
Description:  modified (diff) 
Status:  needs_work → needs_review 
Summary:  new categories for cartesian products with orders → cartesian products with orders 
I've now derived a class from sets.cartesian_product.CartesianProduct
for posets. One can specify by a parameter which order one wants to take. User defined orderes can be used as well.
sage: P = Poset((srange(3), lambda left, right: left <= right)) sage: P.CartesianProduct = sage.sets.cartesian_product.CartesianProductPosets sage: Cl = cartesian_product((P, P), order='lex') sage: Cl((1, 1)) <= Cl((2, 0)) True sage: Cc = cartesian_product((P, P), order='components') sage: Cc((1, 1)) <= Cc((2, 0)) False sage: def le_sum(left, right): ....: return (sum(left) < sum(right) or ....: sum(left) == sum(right) and left[0] <= right[0]) sage: Cs = cartesian_product((P, P), order=le_sum) sage: Cs((1, 1)) <= Cs((2, 0)) True
Last 10 new commits:
7b098f7  parameter extra_category for cartesian products

c6a01f8  allow kwargs in CartesianProduct (to be used in inherited classes)

85f44f3  pass on keyword arguments in cartesian product of the sets category

ba5dab9  update docstring and complete doctests

55a7020  create class for cartesian products of posets

6ed9246  method "le"

4c9f72c  methods for comparing lexicographically and componentwise

e589e9b  create Element: implement <=, <, >=, >

69335cc  write a class description

9352031  change one word in docstring

comment:14 Changed 7 years ago by
comment:15 Changed 7 years ago by
Commit:  9352031b310b81f83c09ecac11b1ce15163f55f9 → ef15d6c3f9f78452897b760fdf6d828353c0b8c1 

Branch pushed to git repo; I updated commit sha1. New commits:
ef15d6c  deal with categories: CartesianProductPosets should be in Posets()

comment:16 Changed 7 years ago by
Description:  modified (diff) 

comment:17 Changed 7 years ago by
Branch:  u/dkrenn/cat/cartesianproductposets → u/behackl/cat/cartesianproductposets 

Commit:  ef15d6c3f9f78452897b760fdf6d828353c0b8c1 → 0690e716b925505dc34b77338af0da8f35fc53a4 
Reviewers:  → Benjamin Hackl 
Status:  needs_review → needs_info 
Hello!
I've started to review this ticket and merged it into 6.9.beta5. Here are some comments:
 The changes in
covariant_functorial_construction.py
look good to me.  Regarding the changes in
categories/sets_cat.py
: I've added a small reviewer commit to improve the language in the docstring of
cartesian_product
.  Overall, these changes also look good to me. I only have one question regarding the
extra_category
keyword: should it be possible to pass lists (or some other iterable) of categories toextra_category
, or is there no use case for that?
 I've added a small reviewer commit to improve the language in the docstring of
As far as I can see, everything else regarding these changes has been discussed on #18586.
I'll report back as soon as I had a look at the remaining changes.
Last 10 new commits:
ba5dab9  update docstring and complete doctests

55a7020  create class for cartesian products of posets

6ed9246  method "le"

4c9f72c  methods for comparing lexicographically and componentwise

e589e9b  create Element: implement <=, <, >=, >

69335cc  write a class description

9352031  change one word in docstring

ef15d6c  deal with categories: CartesianProductPosets should be in Posets()

d34c1fa  Merge branch 'u/dkrenn/cat/cartesianproductposets' of git://trac.sagemath.org/sage into 6.9.beta5

0690e71  improve language in cartesian_product docstring

comment:18 followup: 25 Changed 7 years ago by
Status:  needs_info → needs_work 

Hello,
 I think that the class
CartesianProductPosets
would better be somewhere incombinat/posets
rather insets/cartesian_product
. It is always better to split files.
 Why is the following not a poset
sage: P = Poset((srange(3), lambda left, right: left <= right)) sage: cartesian_product([P,P]).category() Category of Cartesian products of finite enumerated sets
 The argument
order
in the classCartesianProductPosets
is mandatory. While the following just workssage: C = cartesian_product([ZZ, ZZ], extra_category=Posets())
actually notsage: TestSuite(C).run() Failure in _test_not_implemented_methods: Traceback (most recent call last): ... AssertionError: Not implemented method: le  The following tests failed: _test_not_implemented_methods
 As I already said, it would make sense to have the product order as a default since it is the cartesian product (in a categorical sense when morphisms are increasing functions).
Vincent
comment:19 followups: 24 26 Changed 7 years ago by
 Instead of
order=components
I would useorder=product
which is the standard name (see wikipedia). And put it first in your list in view of comment 4.
 Why
super(CartesianProductPosets, self).__init__(sets, category, **kwargs) from sage.categories.posets import Posets self._refine_category_(Posets())
instead ofcategory = category.join(Posets()) CartesianProduct.__init__(sets, category, **kwargs)
I guess that the refine category operation is highly non trivial (perhaps even sloppy).
 (from your doctest) This is ugly
sage: QQ.CartesianProduct = sage.sets.cartesian_product.CartesianProductPosets
Is this the way we are supposed to use to construct cartesian product of posets!?
 Why not implementing
_richcmp_
(see #18305)? You would end up with less code.
 It is always (should always be) faster to use
x <= y
instead ofx.__le__(y)
. You avoid a lookup.
comment:20 Changed 7 years ago by
Commit:  0690e716b925505dc34b77338af0da8f35fc53a4 → 0a74ff134169bd9e1fa9d50eb061f69c19fe351e 

Branch pushed to git repo; I updated commit sha1. New commits:
0a74ff1  typos, language, and punctuation.

comment:21 followup: 22 Changed 7 years ago by
Thanks for having a look at the code Vincent! I'd like to add some more comments as well.
Replying to vdelecroix:
 I think that the class
CartesianProductPosets
would better be somewhere incombinat/posets
rather insets/cartesian_product
. It is always better to split files.
Currently, I like the idea that both "types" of cartesian products can be found in the same file; I'm not quite sure if it would really be more clean to move this code to some other file. In any case, I don't really have any arguments for or against moving the code, I'll leave this decision to those with more experience.
 Why is the following not a poset
sage: P = Poset((srange(3), lambda left, right: left <= right)) sage: cartesian_product([P,P]).category() Category of Cartesian products of finite enumerated sets
This is the same behavior as we had before; this ticket did not change that. However, it probably should.
My suggestion would be to add the line
CartesianProduct = CartesianProductPosets
to the ParentMethods
class in categories/posets.py
, together with an appropriate (lazy) import. If I understood correctly, this would make all parents with a subcategory of Posets()
use the CartesianProductPosets
class for constructing the cartesian product.
This change would also suggest using a default argument for the order (otherwise, this change is potentially backwardsincompatible)and as Vincent has pointed out in 4., this would be somehow natural.
 The argument
order
in the classCartesianProductPosets
is mandatory. While the following just workssage: C = cartesian_product([ZZ, ZZ], extra_category=Posets())actually notsage: TestSuite(C).run() Failure in _test_not_implemented_methods: Traceback (most recent call last): ... AssertionError: Not implemented method: le  The following tests failed: _test_not_implemented_methods
As far as I can see, this comes from
return parents[0].CartesianProduct(...)
in the last line of cartesian_product
in categories/sets_cat.py
. If the category of parents[0]
isn't a subcategory of Posets()
, the nonposet cartesian product is constructed (even with my suggestion from above). In any case, I think it is easy to take care of that case.
I agree completely with 4, 5, 7; and I know too little about _refine_category_
to comment on 6. It seems like a good idea to follow #18305 and implement _richcmp_
in the Element
class.
Finally, I pushed another small commit with some documentationrelated changes.
I'd suggest taking care of setting Parent.CartesianProduct
automatically for subcategories of Posets()
, as well as adapting cartesian_product
such that in case category
is a subcategory of Posets()
, the class CartesianProductPosets
is used for the construction of the cartesian product.
Other than that, this would be positive_review
from my side.
Benjamin
comment:22 Changed 7 years ago by
Hi Benjamin,
Replying to behackl:
Thanks for having a look at the code Vincent! I'd like to add some more comments as well.
Replying to vdelecroix:
 I think that the class
CartesianProductPosets
would better be somewhere incombinat/posets
rather insets/cartesian_product
. It is always better to split files.Currently, I like the idea that both "types" of cartesian products can be found in the same file; I'm not quite sure if it would really be more clean to move this code to some other file. In any case, I don't really have any arguments for or against moving the code, I'll leave this decision to those with more experience.
It is not two disjoint types of CartesianProduct
. One inherits from the other.
Some arguments for the splitting:
 there at least one antecedant:
sage.groups.group_semidirect_product.GroupSemidirectProduct
 if we start populating
sets.cartesian_product
it might become very huge  it is (up to
categories/*
) how the code is organized in the source. If you are looking for features of poset you look insage.combinat.posets
(ideally it would even besage.posets
).
Vincent
comment:23 Changed 7 years ago by
Branch:  u/behackl/cat/cartesianproductposets → u/dkrenn/cat/cartesianproductposets 

comment:24 Changed 7 years ago by
Commit:  0a74ff134169bd9e1fa9d50eb061f69c19fe351e → e87db1a0f635bb81e3510dfa7ca1617e31462380 

Hi Vicent,
Replying to vdelecroix:
 Why not implementing
_richcmp_
(see #18305)? You would end up with less code.
Indeed it would. It seems that element_wrapper
is not using my _richcmp_
and fixing element_wrapper
seems a bit more work (and not suitable for this ticket here).
New commits:
b8a54e6  move posetcode from sets.cartesian_product to combinat.posets.cartesian_product

d316db5  set CartesianProduct in category Posets

3b9a41e  rename order 'components' to 'product'

c0f27c3  define default order=product

e87db1a  rewrite (simplify) join of category

comment:25 Changed 7 years ago by
Replying to vdelecroix:
 I think that the class
CartesianProductPosets
would better be somewhere incombinat/posets
rather insets/cartesian_product
. It is always better to split files.
Ok, split and moved to sage.combinat.posets
 Why is the following not a poset
sage: P = Poset((srange(3), lambda left, right: left <= right)) sage: cartesian_product([P,P]).category() Category of Cartesian products of finite enumerated sets
It now is. (I've used the solution Benjamin suggested above).
 The argument
order
in the classCartesianProductPosets
is mandatory. While the following just workssage: C = cartesian_product([ZZ, ZZ], extra_category=Posets())actually notsage: TestSuite(C).run() Failure in _test_not_implemented_methods: Traceback (most recent call last): ... AssertionError: Not implemented method: le  The following tests failed: _test_not_implemented_methods
The problem is that ZZ
(and QQ
as well) is not a in the category of Posets
. (#19269 will change this).
 As I already said, it would make sense to have the product order as a default since it is the cartesian product (in a categorical sense when morphisms are increasing functions).
Default argument: done.
comment:26 Changed 7 years ago by
Replying to vdelecroix:
 Instead of
order=components
I would useorder=product
which is the standard name (see wikipedia). And put it first in your list in view of comment 4.
Changed.
 Why
super(CartesianProductPosets, self).__init__(sets, category, **kwargs) from sage.categories.posets import Posets self._refine_category_(Posets())instead ofcategory = category.join(Posets()) CartesianProduct.__init__(sets, category, **kwargs)I guess that the refine category operation is highly non trivial (perhaps even sloppy).
Yes, it was kind of sloppy. Changed now.
 (from your doctest) This is ugly
sage: QQ.CartesianProduct = sage.sets.cartesian_product.CartesianProductPosetsIs this the way we are supposed to use to construct cartesian product of posets!?
This changes, once QQ
has category Posets
as well (#19269).
 Why not implementing
_richcmp_
(see #18305)? You would end up with less code.
Skipped; see comment above.
 It is always (should always be) faster to use
x <= y
instead ofx.__le__(y)
. You avoid a lookup.
Ok, thanks for this hint.
comment:27 followups: 30 31 Changed 7 years ago by
Hi Daniel, hi Benjamin,
Thanks for all the modifs. If it is an easy ticket, could we set #19269 as a dependency? The current related doctests are really awful. If it is not that simple, we will fix these in #19269.
Concerning the ElementWrapper
I indeed remember that I had the same trouble when converting some code to the new testing framework.
Vincent
comment:28 Changed 7 years ago by
Commit:  e87db1a0f635bb81e3510dfa7ca1617e31462380 → 6714aaaf749e2618e44fba307c0e0a389a4e73c1 

Branch pushed to git repo; I updated commit sha1. New commits:
6714aaa  add comment pointing to #19269 in doctests using QQ as poset

comment:29 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:30 Changed 7 years ago by
Replying to vdelecroix:
Thanks for all the modifs. If it is an easy ticket, could we set #19269 as a dependency? The current related doctests are really awful. If it is not that simple, we will fix these in #19269.
I'd prefer to fix this in #19269 (due to the discussions there).
comment:31 Changed 7 years ago by
Replying to vdelecroix:
Concerning the
ElementWrapper
I indeed remember that I had the same trouble when converting some code to the new testing framework.
Once these troubles are solved; I've prepared an implementation of _richcmp_
in branch
u/dkrenn/cat/cartesianproductposetsrichcmp
comment:32 followup: 35 Changed 7 years ago by
Status:  needs_review → needs_work 

Instead of using QQ
which is not a poset in the doctest why not use available ones from Posets.<tab>
?
Some more comments
 In the documentation clqss of
CartesianProductPosets
you should say thatorder
andorder_le
are optionals and what is the default behavior. You should also document the behavior ofsage: CartesianProductPoset([X,Y,Z], order='lex', order_le=my_le)
(or raise an appropriate error)
 I am not sure how much this is supported but parents are allowed to have their
category
argument to be set to a tuple as insage: P = Parent(category=(Rings(),Posets())) sage: P.category() Join of Category of rings and Category of posets
In your subclass, the operationcategory & Posets()
would break. You might use the saferCategory.join(my_list_of_categories)
. If you do so, add an appropriate doctest for that.
 What is the point of adding
*kwargs
in the__init__
method ofCartesianProduct
? If this argument is not supported then it should simply not exists.
 Add some
TestSuite(C).run()
for various cartesian products of posets (i.e. at least with the two default implemented orders and one self implemented).
 Since the cartesian product elements are tuples, we might want to use the orders from the tuple themselves (which is fastest then anything else). What about an alternative
def le_native(self, x, y): return x.value <= y.value
 I am not sure that it is good to have the
le_*
methods as being public. Do you have some feeling about it?
comment:33 Changed 7 years ago by
Commit:  6714aaaf749e2618e44fba307c0e0a389a4e73c1 → 693b0bd6b4e5bd6f49afc6db055b066856ae01e5 

comment:34 Changed 7 years ago by
Commit:  693b0bd6b4e5bd6f49afc6db055b066856ae01e5 → f3b1387b378ce406e04c6f45c6629595a46640c8 

Branch pushed to git repo; I updated commit sha1. New commits:
f3b1387  use Posets.ChainPosets in a doctest

comment:35 followup: 36 Changed 7 years ago by
Status:  needs_work → needs_review 

Replying to vdelecroix:
Instead of using
Posets.<tab>
?
Ok, done for the public method(s). (But I've kept the QQ
test in some of the private methods).
 In the documentation clqss of
CartesianProductPosets
you should say thatorder
andorder_le
are optionals and what is the default behavior. You should also document the behavior ofsage: CartesianProductPoset([X,Y,Z], order='lex', order_le=my_le)(or raise an appropriate error)
order_le
is not a keyword. I've rewritten the doc to avoid this confusion.
 I am not sure how much this is supported but parents are allowed to have their
category
argument to be set to a tuple as insage: P = Parent(category=(Rings(),Posets())) sage: P.category() Join of Category of rings and Category of posetsIn your subclass, the operationcategory & Posets()
would break. You might use the saferCategory.join(my_list_of_categories)
. If you do so, add an appropriate doctest for that.
Changed.
 What is the point of adding
*kwargs
in the__init__
method ofCartesianProduct
? If this argument is not supported then it should simply not exists.
This boils down to our discussion from #18586: They are passed on to CartesianProduct
where there is at least a flatten
keyword. Thus there is a keyword, so arguments are simply passed.
 Add some
TestSuite(C).run()
for various cartesian products of posets (i.e. at least with the two default implemented orders and one self implemented).
Added.
 Since the cartesian product elements are tuples, we might want to use the orders from the tuple themselves (which is fastest then anything else). What about an alternative
def le_native(self, x, y): return x.value <= y.value
Added.
 I am not sure that it is good to have the
le_*
methods as being public. Do you have some feeling about it?
Yes, I'd prefer to keep them public, so one can use a different ordering easily (I mean, without changing the given order, but use another order for whatever reasons). Moreover, there seems no need to hide this methods from the public (they can just be used as le
in posets).
comment:36 followup: 38 Changed 7 years ago by
Status:  needs_review → needs_info 

Replying to dkrenn:
Replying to vdelecroix:
 In the documentation <...>
order_le
is not a keyword. I've rewritten the doc to avoid this confusion.
got it ;)
 What is the point of adding
*kwargs
in the__init__
method ofCartesianProduct
? If this argument is not supported then it should simply not exists.This boils down to our discussion from #18586: They are passed on to
CartesianProduct
where there is at least aflatten
keyword. Thus there is a keyword, so arguments are simply passed.
I still do not understand at all. I am talking about sage.sets.cartesian_product.CartesianProduct
. Its constructor does not pass its arguments to anybody. What is the subtle difference in behaviour between f
and g
below?
def f(**kwds): if kwds: raise TypeError ... def g(): ...
The only thing I see is that it needs much more to write f
.
comment:37 Changed 7 years ago by
Commit:  f3b1387b378ce406e04c6f45c6629595a46640c8 → 4ac40147cd398c65cb524a8dfe59f7d5060788c6 

Branch pushed to git repo; I updated commit sha1. New commits:
4ac4014  remove kwargs in CartesianProduct since not needed

comment:38 Changed 7 years ago by
Status:  needs_info → needs_review 

Replying to vdelecroix:
Replying to dkrenn:
 What is the point of adding
*kwargs
in the__init__
method ofCartesianProduct
? If this argument is not supported then it should simply not exists.This boils down to our discussion from #18586: They are passed on to
CartesianProduct
where there is at least aflatten
keyword. Thus there is a keyword, so arguments are simply passed.I still do not understand at all. I am talking about
sage.sets.cartesian_product.CartesianProduct
. Its constructor does not pass its arguments to anybody. What is the subtle difference in behaviour betweenf
andg
below?def f(**kwds): if kwds: raise TypeError ... def g(): ...The only thing I see is that it needs much more to write
f
.
I think I see your point now and I have to admit I thought we were taking about CartesianProductPosets
(and not about CartesianProduct
). I agree that this is obsolete now. I've deleted it.
New commits:
4ac4014  remove kwargs in CartesianProduct since not needed

Last 10 new commits:
e87db1a  rewrite (simplify) join of category

6714aaa  add comment pointing to #19269 in doctests using QQ as poset

298773e  explain keyword order better

08469c7  add module to reference/combinat docs

98c252f  allow tuples as category

eb711e9  add TestSuite

ca4a844  minor rephrase of docstring

693b0bd  add a native order option

f3b1387  use Posets.ChainPosets in a doctest

4ac4014  remove kwargs in CartesianProduct since not needed

comment:40 followup: 41 Changed 7 years ago by
Branch:  u/dkrenn/cat/cartesianproductposets → u/behackl/cat/cartesianproductposets 

Commit:  4ac40147cd398c65cb524a8dfe59f7d5060788c6 → 6ca3e5fd32c7fc5398ece1876ce5981f27663a60 
I fixed the two failing doctests (in 6.9.beta7
the category of the cartesian product of ZZ
with itself changed) and merged 6.9.rc0
into this branch.
Vincent, as you have done quite some work here would you like to add your name to the reviewers?
Finally, as this ticket seems to have reached a stable state, this could be set to positive_review
. Any objections?
Benjamin
Last 10 new commits:
298773e  explain keyword order better

08469c7  add module to reference/combinat docs

98c252f  allow tuples as category

eb711e9  add TestSuite

ca4a844  minor rephrase of docstring

693b0bd  add a native order option

f3b1387  use Posets.ChainPosets in a doctest

4ac4014  remove kwargs in CartesianProduct since not needed

4af1dd4  Merge tag '6.9.rc0' into cat/cartesianproductposets

6ca3e5f  fixed doctests

comment:41 followup: 42 Changed 7 years ago by
Reviewers:  Benjamin Hackl → Benjamin Hackl, Vincent Delecroix 

Replying to behackl:
Vincent, as you have done quite some work here would you like to add your name to the reviewers?
done
Finally, as this ticket seems to have reached a stable state, this could be set to
positive_review
. Any objections?
The branch is in good shape. I would like to go through the whole changes a last time. If everything is fine I will set the positive review. Is that ok?
Vincent
comment:42 Changed 7 years ago by
Replying to vdelecroix:
The branch is in good shape. I would like to go through the whole changes a last time. If everything is fine I will set the positive review. Is that ok?
Sure! Thanks for going over it once again! :)
Benjamin
comment:43 followup: 45 Changed 7 years ago by
Status:  needs_review → needs_work 

 The name
CartesianProductPosets
looks weird. I would rather go forCartesianProductPoset
(nos
) orPosetsCartesianProduct
. As it is currently it looks like the set of posets that are made from cartesian products.
 You should remove your name from
sage/sets/cartesian_product.py
. I have nothing agaist authorship but as it is, it looks like spam ;)
 There is no example testing that the coercion can get involved in comparisons. Either it is worth it and it needs a use case or it can be removed.
 This is not nice
+ CartesianProductPosets = LazyImport( + 'sage.combinat.posets.cartesian_product', 'CartesianProductPosets') + CartesianProduct = CartesianProductPosets
becausesage: E = Posets().example() sage: E.Car<tab> E.CartesianProduct E.CartesianProductPosets
which actually point toward the exact same thing. Please change for+ CartesianProduct = LazyImport( + 'sage.combinat.posets.cartesian_product', 'CartesianProductPosets')
And some questions:
 Most methods of finite posets are actually not available in this class... this is very annoying (but I do not see what can be done at the level of this ticket).
 Are you aware that
sage: C = cartesian_product([ZZ, ZZ], extra_category=Posets()) sage: from sage.combinat.posets.cartesian_product import CartesianProductPosets sage: isinstance(C, CartesianProductPosets) False
and indeedsage: TestSuite(C).run() Failure in _test_not_implemented_methods: ... AssertionError: Not implemented method: le  The following tests failed: _test_not_implemented_methods
comment:44 Changed 7 years ago by
Branch:  u/behackl/cat/cartesianproductposets → u/dkrenn/cat/cartesianproductposets 

comment:45 Changed 7 years ago by
Commit:  6ca3e5fd32c7fc5398ece1876ce5981f27663a60 → 8f9a61942e14254ec028924bccf918774fb57cf6 

Status:  needs_work → needs_review 
Replying to vdelecroix:
 The name
CartesianProductPosets
looks weird. I would rather go forCartesianProductPoset
(nos
) orPosetsCartesianProduct
. As it is currently it looks like the set of posets that are made from cartesian products.
Ok (I don't have an opinion on this); changed to CartesianProductPoset
.
 You should remove your name from
sage/sets/cartesian_product.py
. I have nothing agaist authorship but as it is, it looks like spam ;)
Ooops...forgotten to remove it (when moving to the code to combinat.posets
). Thanks; removed.
 There is no example testing that the coercion can get involved in comparisons. Either it is worth it and it needs a use case or it can be removed.
I've added a doctest. This test can and will be rewritten once #18182 is merged.
 This is not nice
+ CartesianProductPosets = LazyImport( + 'sage.combinat.posets.cartesian_product', 'CartesianProductPosets') + CartesianProduct = CartesianProductPosetsbecausesage: E = Posets().example() sage: E.Car<tab> E.CartesianProduct E.CartesianProductPosetswhich actually point toward the exact same thing.
I completly agree.
Please change for
+ CartesianProduct = LazyImport( + 'sage.combinat.posets.cartesian_product', 'CartesianProductPosets')
I wasn't aware that this works, but it is clear that it works :) Changed.
 Most methods of finite posets are actually not available in this class... this is very annoying (but I do not see what can be done at the level of this ticket).
True; I think this should be fixed by using the Posets
category, but definitely not on this ticket.
 Are you aware that
sage: C = cartesian_product([ZZ, ZZ], extra_category=Posets()) sage: from sage.combinat.posets.cartesian_product import CartesianProductPosets sage: isinstance(C, CartesianProductPosets) Falseand indeedsage: TestSuite(C).run() Failure in _test_not_implemented_methods: ... AssertionError: Not implemented method: le  The following tests failed: _test_not_implemented_methods
Yes, I am aware of. Adding an extra category only makes sense if the underlying object is a poset (thus has le
). Since ZZ
is not a poset (see #19269), the cartesian product is not a poset (and has no le
) and therefore is not an instance of CartesianProductPoset
. Thus, the failure of the test suite is correct.
FYI: The class used for the cartesian product is determined by its first factor (and not by the category).
New commits:
70aa9c4  rename CartesianProductPosets to CartesianProductPoset

f21990d  codesimplify CartesianProduct assignment

8f9a619  add a doctest dealing with coercion while comparing

comment:46 Changed 7 years ago by
Dependencies:  #18586 

Status:  needs_review → positive_review 
All right. Thanks for the last changes.
Vincent
comment:47 Changed 7 years ago by
Branch:  u/dkrenn/cat/cartesianproductposets → 8f9a61942e14254ec028924bccf918774fb57cf6 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
parameter 'category' for cartesian products
parameter extra_category for cartesian products
cartesian products for comparing lexicographically and componentwise (first working draft)
add docstrings and tests
extend __init_extra__