Opened 5 years ago
Closed 5 years ago
#18586 closed enhancement (duplicate)
passing on parameters and extra_category for cartesian products
Reported by: | dkrenn | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | categories | Keywords: | sd67 |
Cc: | behackl, nthiery | Merged in: | |
Authors: | Daniel Krenn | Reviewers: | Daniel Krenn |
Report Upstream: | N/A | Work issues: | |
Branch: | u/dkrenn/cat/extra-category (Commits) | Commit: | ba5dab93e6c7b33eeefc0c937dd2a7d11098807a |
Dependencies: | Stopgaps: |
Description
When calling cartesian_product
(functorial construction), keyword arguments should be passed on to the class constructor of CartesianProduct
. This will allow to use classes inherited from sets.cartesian_product.CartesianProduct? to use these keywords as an input. Additionally it will be possible to add an extra category (to the determined one; instead of only specifying the exact category of the newly created cartesian product)
This ticket arises from #18223; existing code there is partially used here (since the solution there will not be used the usable parts was factored out)
Change History (13)
comment:1 Changed 5 years ago by
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
- Keywords sd67 added
comment:3 follow-up: ↓ 5 Changed 5 years ago by
- Reviewers set to Vincent Delecroix
- Status changed from needs_review to needs_info
comment:4 Changed 5 years ago by
- Milestone changed from sage-6.8 to sage-6.9
comment:5 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 5 years ago by
Replying to vdelecroix:
But what is the point of the ignored
flatten
argument and the forbiddenkwargs
incartesian_product.py
??
It was already there before...I think the idea is that at some point in future, a cartesian product of cartesian products can be flattened, so that this results in a non-nested form of the cartesian product.
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 5 years ago by
Replying to dkrenn:
Replying to vdelecroix:
But what is the point of the ignored
flatten
argument and the forbiddenkwargs
incartesian_product.py
??It was already there before...I think the idea is that at some point in future, a cartesian product of cartesian products can be flattened, so that this results in a non-nested form of the cartesian product.
Would it hurt to remove it? What about the kwargs
that you added?
comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 5 years ago by
Replying to vdelecroix:
But what is the point of the ignored
flatten
argument and the forbiddenkwargs
incartesian_product.py
??Would it hurt to remove it?
I wouldn't mind at all. I have not tested what happens if we do. ...but someone introduced it, maybe this person has an opinion on it?
What about the
kwargs
that you added?
They are not used in sets.cartesian_product.CartesianProduct
but can be used in a derived class (e.g. #18223).
comment:8 in reply to: ↑ 7 ; follow-ups: ↓ 9 ↓ 10 Changed 5 years ago by
Replying to dkrenn:
Replying to vdelecroix:
But what is the point of the ignored
flatten
argument and the forbiddenkwargs
incartesian_product.py
??Would it hurt to remove it?
I wouldn't mind at all. I have not tested what happens if we do. ...but someone introduced it, maybe this person has an opinion on it?
He might. He is Nicolas Thiéry in commit c4629fde
from 2010. But that is not the good way to go. We could add ignored parameters a
, b
, c
, ... they might be very useful in the future.
What about the
kwargs
that you added?They are not used in
sets.cartesian_product.CartesianProduct
but can be used in a derived class (e.g. #18223).
Why not adding it in #18223 then? With this ticket we end up with two arguments that are for future use. It is not an improvement.
comment:9 in reply to: ↑ 8 Changed 5 years ago by
Replying to vdelecroix:
They are not used in
sets.cartesian_product.CartesianProduct
but can be used in a derived class (e.g. #18223).Why not adding it in #18223 then? With this ticket we end up with two arguments that are for future use. It is not an improvement.
The changes here in this ticket are the only changes that are needed in sets.cartesian_product.CartesianProduct
. This here is general enough to be used in various derived classes; it does not have any relation to the content/topic of #18223. Thus separating it and making it its own ticket makes the changes clearer (so that not (silently) a new flag in the general class is introduced).
comment:10 in reply to: ↑ 8 Changed 5 years ago by
- Milestone changed from sage-6.9 to sage-duplicate/invalid/wontfix
- Status changed from needs_info to needs_review
Replying to vdelecroix:
Why not adding it in #18223 then? With this ticket we end up with two arguments that are for future use. It is not an improvement.
Since no comments for a couple of weeks; I'll mark this ticket as "won't fix" and change the description of #18223 so that it will be clear that the changes here are included (since needed there).
Best
Daniel
comment:12 Changed 5 years ago by
- Reviewers set to Daniel Krenn
- Status changed from needs_review to positive_review
comment:13 Changed 5 years ago by
- Resolution set to duplicate
- Status changed from positive_review to closed
Hello,
Your change to
sets_cat.py
looks good to me. But what is the point of the ignoredflatten
argument and the forbiddenkwargs
incartesian_product.py
??Vincent