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 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 hivert 7 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 8 years ago by hivert

  • Authors set to Florent Hivert
  • Description modified (diff)

comment:2 Changed 8 years ago by hivert

  • Status changed from new to needs_review

comment:3 Changed 8 years ago by hivert

  • Owner changed from nthiery to hivert

comment:4 Changed 8 years ago by hivert

  • Description modified (diff)

comment:5 Changed 7 years ago by ncohen

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 7 years ago by ncohen (previous) (diff)

comment:6 Changed 7 years ago by 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 7 years ago by ncohen

  • Status changed from needs_review to needs_work

comment:8 Changed 7 years ago by ncohen

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

Thanks Nathan !!!

comment:10 Changed 7 years ago by jdemeyer

  • Reviewers set to Nathann Cohen

comment:11 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.7.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.