Opened 13 years ago

Closed 5 years ago

#803 closed defect (wontfix)

Tests for what kind of an element something is should test the parent

Reported by: roed Owned by: somebody
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Calls like:

algebras/algebra_order.py:        elif isinstance(x, RingElement) and x in self.base_ring():
algebras/algebra_order_ideal.py:        elif isinstance(x, RingElement) and x in self.base_ring():
algebras/free_algebra_quotient_element.py:        elif isinstance(x, RingElement) and not isinstance(x, AlgebraElement) and x in R:
rings/infinity.py:        elif isinstance(x, RingElement) or isinstance(x, (int,long,float,complex)):
rings/infinity.py:        elif isinstance(x, RingElement):

should actually be checking to see if the parents are of the appropriate type. The element types are not always reliable: parents more accurately reflect the mathematical structure (mostly because they can have multiple inheritance).

There may be more instances in addition to those above (I just ran search_src("isinstance(x, RingElement)"))

Change History (8)

comment:1 Changed 12 years ago by was

I'm dubious:

My immediate thought when looking at #803 is that it is the wrong idea in the first place. If I had looked at #803 before now I would have considered marking it "invalid".

There are individual instances that involve "isinstance" that were perhaps badly written. E.g., the first example:

        elif isinstance(x, RingElement) and x in self.base_ring():
            return True

Here I think the original author (David Kohel) may have written this at a time when "in" could raise an exception if x isn't a RingElement?. That's no longer the case, so in this particular example the right fix is I think to put:

        elif x in self.base_ring():
            return True

In probably hundreds of other cases the use of isinstance in Sage is exactly right. In some cases it could be improved, but how will depend in each case on understanding the code.

comment:2 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:3 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:4 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:6 Changed 5 years ago by mmezzarobba

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Report Upstream set to N/A
  • Status changed from new to needs_review

comment:7 Changed 5 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:8 Changed 5 years ago by vbraun

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.