Opened 5 years ago

Closed 4 years ago

#18223 closed enhancement (fixed)

cartesian products with orders

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-6.7
Component: categories Keywords: sd67
Cc: behackl, cheuberg, nthiery Merged in:
Authors: Daniel Krenn Reviewers: Benjamin Hackl, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 8f9a619 (Commits) Commit: 8f9a61942e14254ec028924bccf918774fb57cf6
Dependencies: Stopgaps:

Description (last modified by dkrenn)

Create cartesian products which have a particular order (lexicographically or component-wise) 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 5 years ago by dkrenn

  • Branch set to u/dkrenn/cat/cartesian

comment:2 Changed 5 years ago by dkrenn

  • Commit set to 46c7a0464379815d0507e7385bb23783c4f7b5f1
  • Status changed from new to needs_review

New commits:

595b1c9parameter 'category' for cartesian products
afaeb0eparameter extra_category for cartesian products
2226fe9cartesian products for comparing lexicographically and component-wise (first working draft)
56eda04add docstrings and tests
46c7a04extend __init_extra__

comment:3 Changed 5 years ago by dkrenn

  • Cc nthiery added

comment:4 follow-up: Changed 4 years ago by chapoton

  • Branch changed from u/dkrenn/cat/cartesian to public/ticket/18223
  • Commit changed from 46c7a0464379815d0507e7385bb23783c4f7b5f1 to 1013cca1a33877e3ba52af111324be937ec19f58

fixed a failing doctest


New commits:

702a8a6Merge branch 'u/dkrenn/cat/cartesian' into 6.7.b1
1013ccatrac #18223 typo in doctest

comment:5 in reply to: ↑ 4 Changed 4 years ago by dkrenn

Replying to chapoton:

fixed a failing doctest

Thanks.

702a8a6Merge branch 'u/dkrenn/cat/cartesian' into 6.7.b1

What was the reason for this merge?

comment:6 follow-up: Changed 4 years ago by 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.

comment:7 in reply to: ↑ 6 Changed 4 years ago by dkrenn

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#merging-and-rebasing 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 4 years ago by git

  • Commit changed from 1013cca1a33877e3ba52af111324be937ec19f58 to b924b94bc2e1aa12021cc70b3a919532aa1e350a

Branch pushed to git repo; I updated commit sha1. New commits:

98fcf3fMerge branch 'public/ticket/18223' into 6.7.b2
b924b94trac #18223 one :: missing

comment:9 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Hello Daniel,

  1. 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.
  1. 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 better
    sage: 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.
  1. 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.
  1. 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 follow-up: Changed 4 years ago by nthiery

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 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by vdelecroix

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 in reply to: ↑ 11 Changed 4 years ago by dkrenn

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 4 years ago by dkrenn

  • Authors set to Daniel Krenn
  • Branch changed from public/ticket/18223 to u/dkrenn/cat/cartesian-product-posets
  • Commit changed from b924b94bc2e1aa12021cc70b3a919532aa1e350a to 9352031b310b81f83c09ecac11b1ce15163f55f9
  • Dependencies set to #18586
  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from new categories for cartesian products with orders to 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:

7b098f7parameter extra_category for cartesian products
c6a01f8allow kwargs in CartesianProduct (to be used in inherited classes)
85f44f3pass on keyword arguments in cartesian product of the sets category
ba5dab9update docstring and complete doctests
55a7020create class for cartesian products of posets
6ed9246method "le"
4c9f72cmethods for comparing lexicographically and component-wise
e589e9bcreate Element: implement <=, <, >=, >
69335ccwrite a class description
9352031change one word in docstring

comment:14 Changed 4 years ago by dkrenn

This new implementation is now used in #18587 for the asymptotic growth groups (#17600, underlying the asymptotic ring #17601)

comment:15 Changed 4 years ago by git

  • Commit changed from 9352031b310b81f83c09ecac11b1ce15163f55f9 to ef15d6c3f9f78452897b760fdf6d828353c0b8c1

Branch pushed to git repo; I updated commit sha1. New commits:

ef15d6cdeal with categories: CartesianProductPosets should be in Posets()

comment:16 Changed 4 years ago by dkrenn

  • Description modified (diff)

comment:17 Changed 4 years ago by behackl

  • Branch changed from u/dkrenn/cat/cartesian-product-posets to u/behackl/cat/cartesian-product-posets
  • Commit changed from ef15d6c3f9f78452897b760fdf6d828353c0b8c1 to 0690e716b925505dc34b77338af0da8f35fc53a4
  • Reviewers set to Benjamin Hackl
  • Status changed from needs_review to 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 to extra_category, or is there no use case for that?

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:

ba5dab9update docstring and complete doctests
55a7020create class for cartesian products of posets
6ed9246method "le"
4c9f72cmethods for comparing lexicographically and component-wise
e589e9bcreate Element: implement <=, <, >=, >
69335ccwrite a class description
9352031change one word in docstring
ef15d6cdeal with categories: CartesianProductPosets should be in Posets()
d34c1faMerge branch 'u/dkrenn/cat/cartesian-product-posets' of git://trac.sagemath.org/sage into 6.9.beta5
0690e71improve language in cartesian_product docstring

comment:18 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_info to needs_work

Hello,

  1. I think that the class CartesianProductPosets would better be somewhere in combinat/posets rather in sets/cartesian_product. It is always better to split files.
  1. 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
    
  1. The argument order in the class CartesianProductPosets is mandatory. While the following just works
    sage: C = cartesian_product([ZZ, ZZ], extra_category=Posets())
    
    actually not
    sage: 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
    
  1. 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

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:19 follow-ups: Changed 4 years ago by vdelecroix

  1. Instead of order=components I would use order=product which is the standard name (see wikipedia). And put it first in your list in view of comment 4.
  1. Why
    super(CartesianProductPosets, self).__init__(sets, category, **kwargs)
    from sage.categories.posets import Posets
    self._refine_category_(Posets())
    
    instead of
    category = category.join(Posets())
    CartesianProduct.__init__(sets, category, **kwargs)
    
    I guess that the refine category operation is highly non trivial (perhaps even sloppy).
  1. (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!?
  1. Why not implementing _richcmp_ (see #18305)? You would end up with less code.
  1. It is always (should always be) faster to use x <= y instead of x.__le__(y). You avoid a lookup.

comment:20 Changed 4 years ago by git

  • Commit changed from 0690e716b925505dc34b77338af0da8f35fc53a4 to 0a74ff134169bd9e1fa9d50eb061f69c19fe351e

Branch pushed to git repo; I updated commit sha1. New commits:

0a74ff1typos, language, and punctuation.

comment:21 follow-up: Changed 4 years ago by behackl

Thanks for having a look at the code Vincent! I'd like to add some more comments as well.

Replying to vdelecroix:

  1. I think that the class CartesianProductPosets would better be somewhere in combinat/posets rather in sets/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.

  1. 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 backwards-incompatible)---and as Vincent has pointed out in 4., this would be somehow natural.

  1. The argument order in the class CartesianProductPosets is mandatory. While the following just works
    sage: C = cartesian_product([ZZ, ZZ], extra_category=Posets())
    
    actually not
    sage: 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 non-poset 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 documentation-related 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

Last edited 4 years ago by behackl (previous) (diff)

comment:22 in reply to: ↑ 21 Changed 4 years ago by vdelecroix

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:

  1. I think that the class CartesianProductPosets would better be somewhere in combinat/posets rather in sets/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 in sage.combinat.posets (ideally it would even be sage.posets).

Vincent

comment:23 Changed 4 years ago by dkrenn

  • Branch changed from u/behackl/cat/cartesian-product-posets to u/dkrenn/cat/cartesian-product-posets

comment:24 in reply to: ↑ 19 Changed 4 years ago by dkrenn

  • Commit changed from 0a74ff134169bd9e1fa9d50eb061f69c19fe351e to e87db1a0f635bb81e3510dfa7ca1617e31462380

Hi Vicent,

Replying to vdelecroix:

  1. 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:

b8a54e6move poset-code from sets.cartesian_product to combinat.posets.cartesian_product
d316db5set CartesianProduct in category Posets
3b9a41erename order 'components' to 'product'
c0f27c3define default order=product
e87db1arewrite (simplify) join of category

comment:25 in reply to: ↑ 18 Changed 4 years ago by dkrenn

Replying to vdelecroix:

  1. I think that the class CartesianProductPosets would better be somewhere in combinat/posets rather in sets/cartesian_product. It is always better to split files.

Ok, split and moved to sage.combinat.posets

  1. 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).

  1. The argument order in the class CartesianProductPosets is mandatory. While the following just works
    sage: C = cartesian_product([ZZ, ZZ], extra_category=Posets())
    
    actually not
    sage: 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).

  1. 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 in reply to: ↑ 19 Changed 4 years ago by dkrenn

Replying to vdelecroix:

  1. Instead of order=components I would use order=product which is the standard name (see wikipedia). And put it first in your list in view of comment 4.

Changed.

  1. Why
    super(CartesianProductPosets, self).__init__(sets, category, **kwargs)
    from sage.categories.posets import Posets
    self._refine_category_(Posets())
    
    instead of
    category = 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.

  1. (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!?

This changes, once QQ has category Posets as well (#19269).

  1. Why not implementing _richcmp_ (see #18305)? You would end up with less code.

Skipped; see comment above.

  1. It is always (should always be) faster to use x <= y instead of x.__le__(y). You avoid a lookup.

Ok, thanks for this hint.

comment:27 follow-ups: Changed 4 years ago by vdelecroix

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 4 years ago by git

  • Commit changed from e87db1a0f635bb81e3510dfa7ca1617e31462380 to 6714aaaf749e2618e44fba307c0e0a389a4e73c1

Branch pushed to git repo; I updated commit sha1. New commits:

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

comment:29 Changed 4 years ago by dkrenn

  • Status changed from needs_work to needs_review

comment:30 in reply to: ↑ 27 Changed 4 years ago by dkrenn

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 in reply to: ↑ 27 Changed 4 years ago by dkrenn

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/cartesian-product-posets-richcmp

comment:32 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to 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

  1. In the documentation clqss of CartesianProductPosets you should say that order and order_le are optionals and what is the default behavior. You should also document the behavior of
    sage: CartesianProductPoset([X,Y,Z], order='lex', order_le=my_le)
    
    (or raise an appropriate error)
  1. 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 in
    sage: P = Parent(category=(Rings(),Posets()))
    sage: P.category()
    Join of Category of rings and Category of posets
    
    In your subclass, the operation category & Posets() would break. You might use the safer Category.join(my_list_of_categories). If you do so, add an appropriate doctest for that.
  1. What is the point of adding *kwargs in the __init__ method of CartesianProduct? If this argument is not supported then it should simply not exists.
  1. 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).
  1. 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
    
  1. 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 4 years ago by git

  • Commit changed from 6714aaaf749e2618e44fba307c0e0a389a4e73c1 to 693b0bd6b4e5bd6f49afc6db055b066856ae01e5

Branch pushed to git repo; I updated commit sha1. New commits:

298773eexplain keyword order better
08469c7add module to reference/combinat docs
98c252fallow tuples as category
eb711e9add TestSuite
ca4a844minor rephrase of docstring
693b0bdadd a native order option

comment:34 Changed 4 years ago by git

  • Commit changed from 693b0bd6b4e5bd6f49afc6db055b066856ae01e5 to f3b1387b378ce406e04c6f45c6629595a46640c8

Branch pushed to git repo; I updated commit sha1. New commits:

f3b1387use Posets.ChainPosets in a doctest

comment:35 in reply to: ↑ 32 ; follow-up: Changed 4 years ago by dkrenn

  • Status changed from needs_work to needs_review

Replying to vdelecroix:

Instead of using QQ which is not a poset in the doctest why not use available ones from Posets.<tab>?

Ok, done for the public method(s). (But I've kept the QQ-test in some of the private methods).

  1. In the documentation clqss of CartesianProductPosets you should say that order and order_le are optionals and what is the default behavior. You should also document the behavior of
    sage: 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.

  1. 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 in
    sage: P = Parent(category=(Rings(),Posets()))
    sage: P.category()
    Join of Category of rings and Category of posets
    
    In your subclass, the operation category & Posets() would break. You might use the safer Category.join(my_list_of_categories). If you do so, add an appropriate doctest for that.

Changed.

  1. What is the point of adding *kwargs in the __init__ method of CartesianProduct? 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.

  1. 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.

  1. 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.

  1. 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 in reply to: ↑ 35 ; follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_info

Replying to dkrenn:

Replying to vdelecroix:

  1. In the documentation <...>

order_le is not a keyword. I've rewritten the doc to avoid this confusion.

got it ;-)

  1. What is the point of adding *kwargs in the __init__ method of CartesianProduct? 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.

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 4 years ago by git

  • Commit changed from f3b1387b378ce406e04c6f45c6629595a46640c8 to 4ac40147cd398c65cb524a8dfe59f7d5060788c6

Branch pushed to git repo; I updated commit sha1. New commits:

4ac4014remove kwargs in CartesianProduct since not needed

comment:38 in reply to: ↑ 36 Changed 4 years ago by dkrenn

  • Status changed from needs_info to needs_review

Replying to vdelecroix:

Replying to dkrenn:

  1. What is the point of adding *kwargs in the __init__ method of CartesianProduct? 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.

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.

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:

4ac4014remove kwargs in CartesianProduct since not needed

Last 10 new commits:

e87db1arewrite (simplify) join of category
6714aaaadd comment pointing to #19269 in doctests using QQ as poset
298773eexplain keyword order better
08469c7add module to reference/combinat docs
98c252fallow tuples as category
eb711e9add TestSuite
ca4a844minor rephrase of docstring
693b0bdadd a native order option
f3b1387use Posets.ChainPosets in a doctest
4ac4014remove kwargs in CartesianProduct since not needed

comment:39 Changed 4 years ago by chapoton

two failing doctests

comment:40 follow-up: Changed 4 years ago by behackl

  • Branch changed from u/dkrenn/cat/cartesian-product-posets to u/behackl/cat/cartesian-product-posets
  • Commit changed from 4ac40147cd398c65cb524a8dfe59f7d5060788c6 to 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:

298773eexplain keyword order better
08469c7add module to reference/combinat docs
98c252fallow tuples as category
eb711e9add TestSuite
ca4a844minor rephrase of docstring
693b0bdadd a native order option
f3b1387use Posets.ChainPosets in a doctest
4ac4014remove kwargs in CartesianProduct since not needed
4af1dd4Merge tag '6.9.rc0' into cat/cartesian-product-posets
6ca3e5ffixed doctests

comment:41 in reply to: ↑ 40 ; follow-up: Changed 4 years ago by vdelecroix

  • Reviewers changed from Benjamin Hackl to 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 in reply to: ↑ 41 Changed 4 years ago by behackl

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 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work
  1. The name CartesianProductPosets looks weird. I would rather go for CartesianProductPoset (no s) or PosetsCartesianProduct. As it is currently it looks like the set of posets that are made from cartesian products.
  1. You should remove your name from sage/sets/cartesian_product.py. I have nothing agaist authorship but as it is, it looks like spam ;-)
  1. 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.
  1. This is not nice
    +        CartesianProductPosets = LazyImport(
    +            'sage.combinat.posets.cartesian_product', 'CartesianProductPosets')
    +        CartesianProduct = CartesianProductPosets
    
    because
    sage: 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:

  1. 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).
  1. 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 indeed
    sage: 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 4 years ago by dkrenn

  • Branch changed from u/behackl/cat/cartesian-product-posets to u/dkrenn/cat/cartesian-product-posets

comment:45 in reply to: ↑ 43 Changed 4 years ago by dkrenn

  • Commit changed from 6ca3e5fd32c7fc5398ece1876ce5981f27663a60 to 8f9a61942e14254ec028924bccf918774fb57cf6
  • Status changed from needs_work to needs_review

Replying to vdelecroix:

  1. The name CartesianProductPosets looks weird. I would rather go for CartesianProductPoset (no s) or PosetsCartesianProduct. 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.

  1. 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.

  1. 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.

  1. This is not nice
    +        CartesianProductPosets = LazyImport(
    +            'sage.combinat.posets.cartesian_product', 'CartesianProductPosets')
    +        CartesianProduct = CartesianProductPosets
    
    because
    sage: E = Posets().example()
    sage: E.Car<tab>
    E.CartesianProduct        E.CartesianProductPosets
    
    which 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.

  1. 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.

  1. 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 indeed
    sage: 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:

70aa9c4rename CartesianProductPosets to CartesianProductPoset
f21990dcode-simplify CartesianProduct assignment
8f9a619add a doctest dealing with coercion while comparing

comment:46 Changed 4 years ago by vdelecroix

  • Dependencies #18586 deleted
  • Status changed from needs_review to positive_review

All right. Thanks for the last changes.

Vincent

comment:47 Changed 4 years ago by vbraun

  • Branch changed from u/dkrenn/cat/cartesian-product-posets to 8f9a61942e14254ec028924bccf918774fb57cf6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.