Opened 4 years ago
Closed 4 years ago
#22382 closed defect (fixed)
Fix facade option for DisjointUnionEnumeratedSets
Reported by:  tscrim  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  categories  Keywords:  
Cc:  jdemeyer, mmezzarobba, nthiery  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Andrew Mathas, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  5c4716b (Commits)  Commit:  5c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02 
Dependencies:  Stopgaps: 
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 4 years ago by
 Branch set to public/sets/disjoint_union_facade22382
 Commit set to 2fdcc12160784b69223ae3fd48ab73388c3f90ce
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit changed from 2fdcc12160784b69223ae3fd48ab73388c3f90ce to 9be48c00d33f4ec27c3a95aaece067c5d04d1381
Branch pushed to git repo; I updated commit sha1. New commits:
9be48c0  Fixing a trivial doctest failure.

comment:3 Changed 4 years ago by
Waiting for the bots to run all of the tests.
comment:4 Changed 4 years ago by
 Commit changed from 9be48c00d33f4ec27c3a95aaece067c5d04d1381 to 26503383caff15cda44a72f36b43b09d929a6776
Branch pushed to git repo; I updated commit sha1. New commits:
2650338  Adding one doctest

comment:5 Changed 4 years ago by
 Reviewers set to Andrew Mathas
Looks good to me. All doctests 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 4 years ago by
 Commit changed from 26503383caff15cda44a72f36b43b09d929a6776 to 82bea355aef7a0749d1bc7f454c9ebc59f67ed10
Branch pushed to git repo; I updated commit sha1. New commits:
82bea35  Formatting tweak and adding a note about the wrapped elements.

comment:7 Changed 4 years ago by
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 sagecombinatdevel. If my changes are good, then positive review.
comment:8 Changed 4 years ago by
 Status changed from needs_review to positive_review
Happy with this. Looks good > positive review.
comment:9 Changed 4 years ago by
Thank you again.
comment:10 Changed 4 years ago by
 Status changed from positive_review to needs_work
comment:11 Changed 4 years ago by
Various test failures
comment:12 Changed 4 years ago by
 Commit changed from 82bea355aef7a0749d1bc7f454c9ebc59f67ed10 to 5c614b79b71ffda5a5dbe9c56c6eb8392d03e7c3
comment:13 Changed 4 years ago by
 Status changed from needs_work to needs_review
The problem comes from the fact that coercion of the E_{6} 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 4 years ago by
still one failing doctest, see bot report
comment:15 Changed 4 years ago by
 Commit changed from 5c614b79b71ffda5a5dbe9c56c6eb8392d03e7c3 to 5c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02
Branch pushed to git repo; I updated commit sha1. New commits:
5c4716b  Fixing doctests missed on previous pass.

comment:16 Changed 4 years ago by
Whoops, thought I got them all...
comment:17 Changed 4 years ago by
 Reviewers changed from Andrew Mathas to Andrew Mathas, Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let it be
comment:18 Changed 4 years ago by
 Branch changed from public/sets/disjoint_union_facade22382 to 5c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Improvements to facades for DisjointUnionEnumeratedSets.