Opened 6 years ago
Closed 6 years ago
#18068 closed defect (fixed)
Fix and simplify comparison of modular forms
Reported by:  pbruin  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  modular forms  Keywords:  modular forms comparison 
Cc:  Merged in:  
Authors:  Peter Bruin  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  7b5e702 (Commits)  Commit:  7b5e70299949ab09cb8bca3397ffedbc3b7e6d93 
Dependencies:  Stopgaps: 
Description (last modified by )
In Sage 6.6.rc1:
sage: f = Newforms(Gamma1(30), 2, names='a')[1] sage: g = loads(dumps(f)) sage: f == g True sage: f != g True # should be False
This is fixed by implementing ModularForm_abstract.__ne__()
.
We also remove the (partial) implementation of __cmp__()
and replace it by __eq__()
and __ne__()
; this simplifies the code and anticipates the removal of cmp()
in Python 3.
Change History (18)
comment:1 Changed 6 years ago by
 Branch set to u/pbruin/18068modular_form_comparison
 Commit set to f65bb00f9f3b5b37637ebb67e0cbefa56026078a
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 6 years ago by
comment:3 Changed 6 years ago by
 Commit changed from f65bb00f9f3b5b37637ebb67e0cbefa56026078a to af4ae72023545ce14eedf32fcdac0d969ef6dd9b
Branch pushed to git repo; I updated commit sha1. New commits:
af4ae72  Trac 18068: simplify comparison methods of modular forms

comment:4 in reply to: ↑ 2 Changed 6 years ago by
Replying to jj:
What about also removing
__cmp__
?
Good point, the last commit simplifies the comparison methods a bit more.
comment:5 Changed 6 years ago by
 Description modified (diff)
 Summary changed from Fix ModularForm_abstract.__ne__() to Fix and simplify comparison of modular forms
comment:6 followup: ↓ 7 Changed 6 years ago by
Hello,
In #17890 Jeroen makes it so that it would be better to implement _richcmp_(left, right, op)
that automatically takes care of coercions. Moreover, inside this function you can assume that left
and right
have the same parent. What do you think?
Vincent
comment:7 in reply to: ↑ 6 Changed 6 years ago by
Hello Vincent,
In #17890 Jeroen makes it so that it would be better to implement
_richcmp_(left, right, op)
that automatically takes care of coercions. Moreover, inside this function you can assume thatleft
andright
have the same parent. What do you think?
That would indeed be good, but wouldn't that need #18305, since we are concerned with Python, not Cython? In that case I'm not sure if I want to wait for #18305 and its dependencies, both because this ticket fixes an actual bug and because it is a dependency of #18061, another bug that I would like to see fixed soon.
comment:8 followup: ↓ 9 Changed 6 years ago by
Sorry, let me make it clearer:
 after #17890 you can implement directly
_richcmp_
or/and_cmp_
to take benefit of coercions in Python classes. If you want to avoid coercions in rich comparisons, you should redefine__eq__
/__ne__
.  #18305 is about cleaning parents implemented as Python classes in the Sage library. For the one which mimic coercion, we just replace the comparisons (either
__cmp__
or__eq__
and friends) with_cmp_
or_richcmp_
.
I have no problem for this ticket to go before #17890. My remark was just informative.
Vincent
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 6 years ago by
Replying to vdelecroix:
Sorry, let me make it clearer:
 after #17890 you can implement directly
_richcmp_
or/and_cmp_
to take benefit of coercions in Python classes. If you want to avoid coercions in rich comparisons, you should redefine__eq__
/__ne__
.
OK, this is what is currently done for modular forms.
I have no problem for this ticket to go before #17890. My remark was just informative.
I spent some time trying to implement _cmp_()
instead of __eq__()
& co., but this was not so easy. It turns out that modular forms do not yet support the coercion model as they should. For example, the forms f
and g
below are in different subspaces of the same ambient space, but this ambient space is not detected as a common parent:
sage: M = ModularForms(Gamma1(13), 2) sage: f = M.cuspidal_submodule().gen(0) sage: g = M.old_submodule()(0) sage: f.parent().ambient() is g.parent().ambient() True sage: f + g Traceback (most recent call last): ... TypeError: unsupported operand parent(s) for '+': 'Cuspidal subspace of dimension 2 of Modular Forms space of dimension 13 for Congruence Subgroup Gamma1(13) of weight 2 over Rational Field' and 'Modular Forms subspace of dimension 0 of Modular Forms space of dimension 13 for Congruence Subgroup Gamma1(13) of weight 2 over Rational Field'
I think it is better to do that on a different ticket.
comment:10 in reply to: ↑ 9 Changed 6 years ago by
Replying to pbruin:
Replying to vdelecroix: ... I think it is better to do that on a different ticket.
Agreed
comment:11 followup: ↓ 14 Changed 6 years ago by
Hi Peter,
Could you mention this ticket number in the documentation, something like this used to fail (see :trac:`18068`)
?
Then it will be good to go.
Vincent
comment:12 Changed 6 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to needs_work
comment:13 Changed 6 years ago by
 Commit changed from af4ae72023545ce14eedf32fcdac0d969ef6dd9b to 451c49de98a9f7b1159503d777675c26917666a2
Branch pushed to git repo; I updated commit sha1. New commits:
451c49d  Trac 18068: add comment about bug fixed by this ticket

comment:14 in reply to: ↑ 11 Changed 6 years ago by
 Status changed from needs_work to positive_review
Replying to vdelecroix:
Could you mention this ticket number in the documentation, something like
this used to fail (see :trac:`18068`)
?
Done in the last commit.
Then it will be good to go.
OK, thanks for the review!
comment:15 followup: ↓ 17 Changed 6 years ago by
 Status changed from positive_review to needs_work
 TESTS:: + TESTS: The following used to fail (see :trac:`18068`)::
comment:16 Changed 6 years ago by
 Commit changed from 451c49de98a9f7b1159503d777675c26917666a2 to 7b5e70299949ab09cb8bca3397ffedbc3b7e6d93
Branch pushed to git repo; I updated commit sha1. New commits:
7b5e702  Trac 18068: fix typo

comment:17 in reply to: ↑ 15 Changed 6 years ago by
 Status changed from needs_work to positive_review
Replying to vdelecroix:
 TESTS:: + TESTS: The following used to fail (see :trac:`18068`)::
Sorry about that, thanks for noticing!
comment:18 Changed 6 years ago by
 Branch changed from u/pbruin/18068modular_form_comparison to 7b5e70299949ab09cb8bca3397ffedbc3b7e6d93
 Resolution set to fixed
 Status changed from positive_review to closed
While you're at it:
__cmp__
doesn't make any sense at all to me (see the except part):Also
__cmp__
is getting deprecated and "<,<=,>,>=" aren't needed anyway for elements I assume. What about also removing__cmp__
?