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 rws)

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 rws

  • Branch set to u/rws/make_realset_answer_positive_to_is_set

comment:2 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 706c762cf622372b74d956708029c48005385a5a
  • Status changed from new to needs_review

New commits:

706c76224443: Make RealSet answer positive to is_Set

comment:3 Changed 4 years ago by vdelecroix

  • 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: Changed 4 years ago by rws

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 mantepse

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: Changed 4 years ago by vdelecroix

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 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?

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 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 rws

  • Authors Ralf Stephan deleted
  • 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 rws

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 rws

  • Branch set to u/rws/24443

comment:10 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to c136a7c6fe4e5622aed06c4d1290940244a2c158
  • Status changed from needs_info to needs_review

New commits:

c136a7c24443: replace is_Set

comment:11 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

deprecation needed

comment:12 Changed 4 years ago by git

  • Commit changed from c136a7c6fe4e5622aed06c4d1290940244a2c158 to 6972d8df44665405e51aca99ad7d74d90a97d77f

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

6972d8d24443: deprecation

comment:13 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:14 Changed 4 years ago by vdelecroix

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

  • Status changed from needs_review to positive_review

comment:16 Changed 4 years ago by vbraun

  • Branch changed from u/rws/24443 to 6972d8df44665405e51aca99ad7d74d90a97d77f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.