Opened 11 years ago

Closed 10 years ago

#12510 closed enhancement (fixed)

Add consistency tests for __nonzero__ in TestSuite.

Reported by: Florent Hivert Owned by: Florent Hivert
Priority: major Milestone: sage-5.7
Component: categories Keywords:
Cc: Christian Stump, Sage Combinat CC user Merged in: sage-5.7.beta1
Authors: Florent Hivert Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Florent Hivert)

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)

trac_12510-nonzero_equal_consistency-fh.patch (18.6 KB) - added by Florent Hivert 10 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by Florent Hivert

Authors: Florent Hivert
Description: modified (diff)

comment:2 Changed 11 years ago by Florent Hivert

Status: newneeds_review

comment:3 Changed 11 years ago by Florent Hivert

Owner: changed from Nicolas M. Thiéry to Florent Hivert

comment:4 Changed 11 years ago by Florent Hivert

Description: modified (diff)

comment:5 Changed 10 years ago by Nathann Cohen

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 problem

sage: a = e.lift()
sage: any([e])
True
sage: b = e.lift()
# Now b and a are not necessarily equal anymore

If you think that this is not a problem after all, then you can set this ticket to positive_review.

Have fuuuuuuuuuuuuuuuuuuuuuuuunnn !!

Nathann

Last edited 10 years ago by Nathann Cohen (previous) (diff)

comment:6 Changed 10 years ago by Florent Hivert

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 10 years ago by Nathann Cohen

Status: needs_reviewneeds_work

Changed 10 years ago by Florent Hivert

comment:8 Changed 10 years ago by Nathann Cohen

Status: needs_workpositive_review

Gooooooooooooooooooooooood to go !!!

Le jury apprécie : la qualité des workarounds :-D

Nathnan

comment:9 Changed 10 years ago by Florent Hivert

Thanks Nathan !!!

comment:10 Changed 10 years ago by Jeroen Demeyer

Reviewers: Nathann Cohen

comment:11 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.7.beta1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.