Opened 4 years ago

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

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by dkrenn

  • Keywords sd67 added

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

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_info

Hello,

Your change to sets_cat.py looks good to me. But what is the point of the ignored flatten argument and the forbidden kwargs in cartesian_product.py??

Vincent

comment:4 Changed 4 years ago by vdelecroix

  • Milestone changed from sage-6.8 to sage-6.9

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

Replying to vdelecroix:

But what is the point of the ignored flatten argument and the forbidden kwargs in cartesian_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: Changed 4 years ago by vdelecroix

Replying to dkrenn:

Replying to vdelecroix:

But what is the point of the ignored flatten argument and the forbidden kwargs in cartesian_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: Changed 4 years ago by dkrenn

Replying to vdelecroix:

But what is the point of the ignored flatten argument and the forbidden kwargs in cartesian_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: Changed 4 years ago by vdelecroix

Replying to dkrenn:

Replying to vdelecroix:

But what is the point of the ignored flatten argument and the forbidden kwargs in cartesian_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 4 years ago by dkrenn

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

  • 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:11 Changed 4 years ago by vdelecroix

  • Reviewers Vincent Delecroix deleted

So let me not review it.

comment:12 Changed 4 years ago by dkrenn

  • Reviewers set to Daniel Krenn
  • Status changed from needs_review to positive_review

comment:13 Changed 4 years ago by vbraun

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.