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: sage-6.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 pbruin)

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 pbruin

  • Branch set to u/pbruin/18068-modular_form_comparison
  • Commit set to f65bb00f9f3b5b37637ebb67e0cbefa56026078a
  • Status changed from new to needs_review

comment:2 follow-up: Changed 6 years ago by jj

While you're at it:

__cmp__ doesn't make any sense at all to me (see the except part):

        try:
            self._ensure_is_compatible(other)
        except Exception:
            return self.parent().__cmp__(other.parent())
        if self.element() == other.element():
            return 0
        else:
            return -1

Also __cmp__ is getting deprecated and "<,<=,>,>=" aren't needed anyway for elements I assume. What about also removing __cmp__?

comment:3 Changed 6 years ago by git

  • Commit changed from f65bb00f9f3b5b37637ebb67e0cbefa56026078a to af4ae72023545ce14eedf32fcdac0d969ef6dd9b

Branch pushed to git repo; I updated commit sha1. New commits:

af4ae72Trac 18068: simplify comparison methods of modular forms

comment:4 in reply to: ↑ 2 Changed 6 years ago by pbruin

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 pbruin

  • Description modified (diff)
  • Summary changed from Fix ModularForm_abstract.__ne__() to Fix and simplify comparison of modular forms

comment:6 follow-up: Changed 6 years ago by vdelecroix

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 pbruin

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 that left and right 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 follow-up: Changed 6 years ago by 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__.
  • #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 ; follow-up: Changed 6 years ago by pbruin

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 vdelecroix

Replying to pbruin:

Replying to vdelecroix: ... I think it is better to do that on a different ticket.

Agreed

comment:11 follow-up: Changed 6 years ago by vdelecroix

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 vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

comment:13 Changed 6 years ago by git

  • Commit changed from af4ae72023545ce14eedf32fcdac0d969ef6dd9b to 451c49de98a9f7b1159503d777675c26917666a2

Branch pushed to git repo; I updated commit sha1. New commits:

451c49dTrac 18068: add comment about bug fixed by this ticket

comment:14 in reply to: ↑ 11 Changed 6 years ago by pbruin

  • 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 follow-up: Changed 6 years ago by vdelecroix

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

  • Commit changed from 451c49de98a9f7b1159503d777675c26917666a2 to 7b5e70299949ab09cb8bca3397ffedbc3b7e6d93

Branch pushed to git repo; I updated commit sha1. New commits:

7b5e702Trac 18068: fix typo

comment:17 in reply to: ↑ 15 Changed 6 years ago by pbruin

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

  • Branch changed from u/pbruin/18068-modular_form_comparison to 7b5e70299949ab09cb8bca3397ffedbc3b7e6d93
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.