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: sage-7.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 tscrim

  • Branch set to public/sets/disjoint_union_facade-22382
  • Commit set to 2fdcc12160784b69223ae3fd48ab73388c3f90ce
  • Status changed from new to needs_review

New commits:

2fdcc12Improvements to facades for DisjointUnionEnumeratedSets.

comment:2 Changed 4 years ago by git

  • Commit changed from 2fdcc12160784b69223ae3fd48ab73388c3f90ce to 9be48c00d33f4ec27c3a95aaece067c5d04d1381

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

9be48c0Fixing a trivial doctest failure.

comment:3 Changed 4 years ago by tscrim

Waiting for the bots to run all of the tests.

comment:4 Changed 4 years ago by git

  • Commit changed from 9be48c00d33f4ec27c3a95aaece067c5d04d1381 to 26503383caff15cda44a72f36b43b09d929a6776

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

2650338Adding one doctest

comment:5 Changed 4 years ago by andrew.mathas

  • Reviewers set to 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 4 years ago by git

  • Commit changed from 26503383caff15cda44a72f36b43b09d929a6776 to 82bea355aef7a0749d1bc7f454c9ebc59f67ed10

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

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

comment:7 Changed 4 years ago by tscrim

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 4 years ago by andrew.mathas

  • Status changed from needs_review to positive_review

Happy with this. Looks good -> positive review.

comment:9 Changed 4 years ago by tscrim

Thank you again.

comment:10 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:11 Changed 4 years ago by vbraun

Various test failures

comment:12 Changed 4 years ago by git

  • Commit changed from 82bea355aef7a0749d1bc7f454c9ebc59f67ed10 to 5c614b79b71ffda5a5dbe9c56c6eb8392d03e7c3

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

  • Status changed from needs_work to needs_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 4 years ago by chapoton

still one failing doctest, see bot report

comment:15 Changed 4 years ago by git

  • Commit changed from 5c614b79b71ffda5a5dbe9c56c6eb8392d03e7c3 to 5c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02

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

5c4716bFixing doctests missed on previous pass.

comment:16 Changed 4 years ago by tscrim

Whoops, thought I got them all...

comment:17 Changed 4 years ago by chapoton

  • 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 vbraun

  • Branch changed from public/sets/disjoint_union_facade-22382 to 5c4716b23c12cd5fe929fd0bfe7370f4aeaf0e02
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.