Opened 4 years ago
Closed 4 years ago
#24443 closed enhancement (fixed)
Replace is_Set() usage
| Reported by: | rws | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | sage-8.2 |
| Component: | basic arithmetic | Keywords: | |
| Cc: | Merged in: | ||
| Authors: | Ralf Stephan | Reviewers: | Vincent Delecroix |
| Report Upstream: | N/A | Work issues: | |
| Branch: | 6972d8d (Commits, GitHub, GitLab) | Commit: | 6972d8df44665405e51aca99ad7d74d90a97d77f |
| Dependencies: | Stopgaps: |
Status badges
Description (last modified by )
Replace calls to is_Set with the code it has, which is just isinstance(x, Set_generic).
Change History (16)
comment:1 Changed 4 years ago by
- Branch set to u/rws/make_realset_answer_positive_to_is_set
comment:2 Changed 4 years ago by
- Commit set to 706c762cf622372b74d956708029c48005385a5a
- Status changed from new to needs_review
comment:3 Changed 4 years ago by
- Status changed from needs_review to needs_info
is_Set is not intended to answer the question "is this object a set". There is no need to let RealSet inherits from Set_generic. As I already mentioned in #15344, what does work is
sage: RealSet(1,2) in Sets() True
comment:4 follow-up: ↓ 6 Changed 4 years ago by
In that case I'd like to either rename is_Set or replace calls to it with the code it has, which is just isinstance(x, Set_generic). Outside of sage/sets grep finds the following usages of which I'm not sure if they are even correct:
src/sage/combinat/set_partition.py:from sage.sets.set import Set, is_Set src/sage/combinat/set_partition.py: if not (isinstance(x, (SetPartition, set, frozenset)) or is_Set(x)): src/sage/combinat/set_partition.py: if not (isinstance(s, (set, frozenset)) or is_Set(s)): src/sage/combinat/set_partition_ordered.py:from sage.sets.set import Set, is_Set src/sage/combinat/set_partition_ordered.py: if not isinstance(s, (set, frozenset)) and not is_Set(s): src/sage/combinat/partition_algebra.py:from sage.sets.set import Set, is_Set src/sage/combinat/partition_algebra.py: assert isinstance(s, (set, frozenset)) or is_Set(s)
What do you think?
comment:5 Changed 4 years ago by
At least from what I understand, these calls to is_Set are correct, and I agree that
isinstance(x, (SetPartition, set, frozenset, Set_generic))
would be easier to understand for me.
More precisely: I believe that this code is to make
sage: Set([Set([1,2])]) in SetPartitions True
work.
comment:6 in reply to: ↑ 4 ; follow-up: ↓ 8 Changed 4 years ago by
Replying to rws:
In that case I'd like to either rename
is_Setor replace calls to it with the code it has, which is justisinstance(x, Set_generic). Outside ofsage/setsgrep finds the following usages of which I'm not sure if they are even correct:src/sage/combinat/set_partition.py:from sage.sets.set import Set, is_Set src/sage/combinat/set_partition.py: if not (isinstance(x, (SetPartition, set, frozenset)) or is_Set(x)): src/sage/combinat/set_partition.py: if not (isinstance(s, (set, frozenset)) or is_Set(s)): src/sage/combinat/set_partition_ordered.py:from sage.sets.set import Set, is_Set src/sage/combinat/set_partition_ordered.py: if not isinstance(s, (set, frozenset)) and not is_Set(s): src/sage/combinat/partition_algebra.py:from sage.sets.set import Set, is_Set src/sage/combinat/partition_algebra.py: assert isinstance(s, (set, frozenset)) or is_Set(s)What do you think?
It has started long ago
sage.schemes.projective.projective_space.is_ProjectiveSpacesage.schemes.product_projective.space.is_ProductProjectiveSpacessage.rings.polynomial.polynomial_ring.is_PolynomialRingsage.rings.ring.is_Ring- etc
The only reasonable solution is to agree with all other developers (on sage-devel) that we want to remove them all from Sage (I am +1 for it). Then we could start with a "task ticket" listing all the is_X and start concretely by removing is_Set.
comment:7 Changed 4 years ago by
- Branch u/rws/make_realset_answer_positive_to_is_set deleted
- Commit 706c762cf622372b74d956708029c48005385a5a deleted
- Description modified (diff)
- Summary changed from Make RealSet answer positive to is_Set to Replace is_Set() usage
- Type changed from defect to task
comment:8 in reply to: ↑ 6 Changed 4 years ago by
Replying to vdelecroix:
The only reasonable solution is to agree with all other developers (on sage-devel) that we want to remove them all from Sage (I am +1 for it).
The question was asked at https://groups.google.com/forum/#!topic/sage-devel/wEzb0awmyN0 and I think there was agreement to do it case by case.
comment:9 Changed 4 years ago by
- Branch set to u/rws/24443
comment:10 Changed 4 years ago by
- Commit set to c136a7c6fe4e5622aed06c4d1290940244a2c158
- Status changed from needs_info to needs_review
New commits:
| c136a7c | 24443: replace is_Set
|
comment:12 Changed 4 years ago by
- Commit changed from c136a7c6fe4e5622aed06c4d1290940244a2c158 to 6972d8df44665405e51aca99ad7d74d90a97d77f
Branch pushed to git repo; I updated commit sha1. New commits:
| 6972d8d | 24443: deprecation
|
comment:13 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 4 years ago by
- Reviewers set to Vincent Delecroix
- Type changed from task to enhancement
Task ticket are "abstract" tickets that are organizing the works of other tickets (and hence containing no branch).
comment:15 Changed 4 years ago by
- Status changed from needs_review to positive_review
comment:16 Changed 4 years ago by
- Branch changed from u/rws/24443 to 6972d8df44665405e51aca99ad7d74d90a97d77f
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
24443: Make RealSet answer positive to is_Set