Opened 4 years ago
Closed 4 years ago
#24443 closed enhancement (fixed)
Replace is_Set() usage
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage8.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: 
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 followup: ↓ 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 ; followup: ↓ 8 Changed 4 years ago by
Replying to rws:
In that case I'd like to either rename
is_Set
or replace calls to it with the code it has, which is justisinstance(x, Set_generic)
. Outside ofsage/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?
It has started long ago
sage.schemes.projective.projective_space.is_ProjectiveSpace
sage.schemes.product_projective.space.is_ProductProjectiveSpaces
sage.rings.polynomial.polynomial_ring.is_PolynomialRing
sage.rings.ring.is_Ring
 etc
The only reasonable solution is to agree with all other developers (on sagedevel) 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 sagedevel) 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/sagedevel/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