Opened 8 years ago
Closed 7 years ago
#12510 closed enhancement (fixed)
Add consistency tests for __nonzero__ in TestSuite.
Reported by: | hivert | Owned by: | hivert |
---|---|---|---|
Priority: | major | Milestone: | sage-5.7 |
Component: | categories | Keywords: | |
Cc: | stumpc5, sage-combinat | Merged in: | sage-5.7.beta1 |
Authors: | Florent Hivert | Reviewers: | Nathann Cohen |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
With many datastructure for elements, comparison to zero could be faster than
comparison of two elements. The pythonic way to do that is to use indirectly
e.__nonzero__()
for example in if not e: ...
. However, to be able
to use consistently this idiom, we have to make sure that the return value of
e == e.parent().zero()
agrees with bool(e)
. The purpose of this
patch is to add this tests to the standard test suite and to fixe the Sage
library according to this policy.
On the way, I discovered that for a some morphism of modules, pickling can be broken if the zero morphism is asked. I also fixes this issue.
See also discussion on #12508. Finally, I left to #12526 the question of removing __nonzero__
from element and writing tuned implantation for all data structures.
Attachments (1)
Change History (12)
comment:1 Changed 8 years ago by
- Description modified (diff)
comment:2 Changed 8 years ago by
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
- Owner changed from nthiery to hivert
comment:4 Changed 8 years ago by
- Description modified (diff)
comment:5 Changed 7 years ago by
comment:6 Changed 7 years ago by
The returned result was wrong so I switched back to raising not implemented. I created #13999 to have the problem fixed. Please review,
Florent
comment:7 Changed 7 years ago by
- Status changed from needs_review to needs_work
Changed 7 years ago by
comment:8 Changed 7 years ago by
- Status changed from needs_work to positive_review
Gooooooooooooooooooooooood to go !!!
Le jury apprécie : la qualité des workarounds :-D
Nathnan
comment:9 Changed 7 years ago by
Thanks Nathan !!!
comment:10 Changed 7 years ago by
- Reviewers set to Nathann Cohen
comment:11 Changed 7 years ago by
- Merged in set to sage-5.7.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Helloooooooo !!
Well, as far as I understand this code I think that it is good to go, except for this thing I just told you about : the new content of
QuotientRingElement.__nonzero__
creates this problemIf you think that this is not a problem after all, then you can set this ticket to
positive_review
.Have fuuuuuuuuuuuuuuuuuuuuuuuunnn !!
Nathann