Opened 6 years ago

Closed 6 years ago

#22382 closed defect (fixed)

Fix facade option for DisjointUnionEnumeratedSets

Reported by: Travis Scrimshaw Owned by:
Priority: major Milestone: sage-7.6
Component: categories Keywords:
Cc: Jeroen Demeyer, Marc Mezzarobba, Nicolas M. Thiéry Merged in:
Authors: Travis Scrimshaw Reviewers: Andrew Mathas, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 5c4716b (Commits, GitHub, GitLab) Commit: 5c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02
Dependencies: Stopgaps:

Status badges

Description

Currently, DisjointUnionEnumeratedSets has an option to be a facade parent, but the category framework, and hence the coercion framework, does not know it is a facade parent. This can lead to test suite failures and some other unexpected behaviors, such as not being able to create elements. In particular, this causes problems noted on #22029.

Change History (18)

comment:1 Changed 6 years ago by Travis Scrimshaw

Branch: public/sets/disjoint_union_facade-22382
Commit: 2fdcc12160784b69223ae3fd48ab73388c3f90ce
Status: newneeds_review

New commits:

2fdcc12Improvements to facades for DisjointUnionEnumeratedSets.

comment:2 Changed 6 years ago by git

Commit: 2fdcc12160784b69223ae3fd48ab73388c3f90ce9be48c00d33f4ec27c3a95aaece067c5d04d1381

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

9be48c0Fixing a trivial doctest failure.

comment:3 Changed 6 years ago by Travis Scrimshaw

Waiting for the bots to run all of the tests.

comment:4 Changed 6 years ago by git

Commit: 9be48c00d33f4ec27c3a95aaece067c5d04d138126503383caff15cda44a72f36b43b09d929a6776

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

2650338Adding one doctest

comment:5 Changed 6 years ago by Andrew Mathas

Reviewers: Andrew Mathas

Looks good to me. All doc-tests past. I added one more doctest, which shows that this patch fixes an issue that I care about, and a missing indirect doctest tag to stop sage -coverage from complaining. I have not checked that the documentation builds but this should be OK because the changes only affect underscore methods. (There is no need to change the documentation because this patch ensures that DisjointUnionEnumeratedSets behaves as it is supposed to.)

If you are happy with my changes then please set to a positive review.

comment:6 Changed 6 years ago by git

Commit: 26503383caff15cda44a72f36b43b09d929a677682bea355aef7a0749d1bc7f454c9ebc59f67ed10

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

82bea35Formatting tweak and adding a note about the wrapped elements.

comment:7 Changed 6 years ago by Travis Scrimshaw

Thank you for looking at this. I made some formatting corrections to your changes, and I added a quick note about the wrapped elements from our discussion on sage-combinat-devel. If my changes are good, then positive review.

comment:8 Changed 6 years ago by Andrew Mathas

Status: needs_reviewpositive_review

Happy with this. Looks good -> positive review.

comment:9 Changed 6 years ago by Travis Scrimshaw

Thank you again.

comment:10 Changed 6 years ago by Volker Braun

Status: positive_reviewneeds_work

comment:11 Changed 6 years ago by Volker Braun

Various test failures

comment:12 Changed 6 years ago by git

Commit: 82bea355aef7a0749d1bc7f454c9ebc59f67ed105c614b79b71ffda5a5dbe9c56c6eb8392d03e7c3

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

c93045dMerge branch 'public/sets/disjoint_union_facade-22382' of git://trac.sagemath.org/sage into public/sets/disjoint_union_facade-22382
5c614b7Don't try coercion unless we have to.

comment:13 Changed 6 years ago by Travis Scrimshaw

Status: needs_workneeds_review

The problem comes from the fact that coercion of the E6 crystal is a bit too permissive. However, properly fixing this issue would likely be expensive in terms of computations, which I don't really want to do at this stage. Instead, the workaround is actually something we should do IMO: test if the element's parents is one of the parents used in the construction.

comment:14 Changed 6 years ago by Frédéric Chapoton

still one failing doctest, see bot report

comment:15 Changed 6 years ago by git

Commit: 5c614b79b71ffda5a5dbe9c56c6eb8392d03e7c35c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02

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

5c4716bFixing doctests missed on previous pass.

comment:16 Changed 6 years ago by Travis Scrimshaw

Whoops, thought I got them all...

comment:17 Changed 6 years ago by Frédéric Chapoton

Reviewers: Andrew MathasAndrew Mathas, Frédéric Chapoton
Status: needs_reviewpositive_review

ok, let it be

comment:18 Changed 6 years ago by Volker Braun

Branch: public/sets/disjoint_union_facade-223825c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.