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
comment:2 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:3 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:4 Changed 6 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:5 Changed 6 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:6 Changed 5 years ago by
- 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
- Reviewers set to Vincent Delecroix
- Status changed from needs_review to positive_review
comment:8 Changed 5 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
Note: See
TracTickets for help on using
tickets.
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:
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:
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.