#31559 closed enhancement (fixed)

Make ModularFormRings manipulate formal objects

Reported by: Vincent Delecroix Owned by: David Ayotte
Priority: major Milestone: sage-9.5
Component: modular forms Keywords: gsoc2021, modular forms rings
Cc: David Ayotte Merged in:
Authors: David Ayotte Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 088139a (Commits, GitHub, GitLab) Commit: 088139ad5f9299a129368cdb8543848ab6dcca5c
Dependencies: #32168 Stopgaps:

Status badges

Description (last modified by Vincent Delecroix)

Currently, ModularFormRings is manipulating truncated q-series. It is desirable to be able to manipulate the ring of modular forms. In other words, being able to define E4 + E6 and access its homogeneous components.

This ticket is part of #31560

Change History (85)

comment:1 Changed 20 months ago by Matthias Köppe

Milestone: sage-9.3sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

comment:2 Changed 18 months ago by Vincent Delecroix

Description: modified (diff)

comment:3 Changed 18 months ago by David Ayotte

Branch: u/gh-DavidAyotte/make_modularformrings_manipulate_formal_objects

comment:4 Changed 18 months ago by David Ayotte

Commit: 5f23e4db95190358b0203af006ca36b0e9973b25
Owner: set to David Ayotte

New commits:

5f23e4dInitial implementation of GradedModularFormElement. The class ModularFormsRing now inherit from Parent and UniqueRepresentation

comment:5 Changed 18 months ago by git

Commit: 5f23e4db95190358b0203af006ca36b0e9973b2533a8cd5deb9ae0a133461af2301044ceb3f01ab6

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

33a8cd5Added the methods _neq_ and is_homogeneous.

comment:6 Changed 18 months ago by git

Commit: 33a8cd5deb9ae0a133461af2301044ceb3f01ab66faa4baaccaf4f6dd5f6fc9b59aa29706f8d8c0b

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

6faa4baImplemented coercion. Added documentation. Fixed a bug in _add_ method.

comment:7 Changed 18 months ago by git

Commit: 6faa4baaccaf4f6dd5f6fc9b59aa29706f8d8c0b9216db9ac8697c0b49d419bdf7c49aee69c8fe41

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

9216db9Moved the class GradedModularFormElement into modular/modform/element.py. Fixed some issues with coercion.

comment:8 Changed 18 months ago by Vincent Delecroix

Authors: David Ayotte
Reviewers: Vincent Delecroix

comment:9 Changed 18 months ago by Vincent Delecroix

In the constructor I would either allow a list of modular forms or a dictionary. In both situations there should be a check that the data is consistent. And in the case it is a dictionary, you would better not write

    self.__mf_dict = mf_dict

because this uses the dictionary provided in the input. This dictionary can be modified outside of the scope of the class and corrupt the data in a scenario such as

sage: d = {}
sage: f = M(d)
sage: d[3] = 'hello'  # data corruption

Why do you have a method _dictionary and not use directly __mf_dict. This add one level of indirection for no purpose that I can see.

When you iterate over the keys and values of a dictionary it is best to use the items() method as in

def __neg__(self):
    GM = self.__class__
    minus_self = {k:-v for k,v in self.__mf_dict.items()}
    return GM(self.parent(), minus_self)

You should always take care of removing the entries in the dictionary whose values are zero (in the constructor and after a binary operation). Otherwise a method such as is_homogeneous gets completely wrong.

Use __neg__ and not _neg_ (this method does not go through corecion).

In __getitem__ you would better return the zero of the base ring and not the python integer 0.

Don't write x.__getitem__(k) but x[k]. The latter is much faster in general.

Rather than

def weights(self):
    wts = list(self._dictionnary().keys())
    wts.sort()
    return wts

you could use

def weights(self):
    return sorted(self.__mf_dict)

Instead of

def is_homogeneous(self):
    if len(self._dictionnary())>1:
        return False
    return True

you could use

def is_homogeneous(self):
    return len(self.__mf_dict) <= 1

For the documentation, it is INPUT and not INPUTS. And the items afterwards should not be indented.

It would be better to inherit from ModuleElement (not doing this causes trouble with the coercion model). Once done, implement the method _lmul_ for scalar multiplications (it should be possible to do integer x (element of the ring of modular forms).

Another method that is lacking is __bool__ (for the zero test). Once the dictionary is clean (ie no zero value) you could simply do

def __bool__(self):
    return bool(self.__mf_dict)

Also write methods is_zero and is_one that are standard ones for rings.

comment:10 Changed 18 months ago by Vincent Delecroix

If qexp is simply an alias of q_expansion write qexp = q_expansion. The documentation of q_expansion could mention the fact that such an alias exists.

The method _repr_ could be simplified to

def _repr_(self):
    return str(self.q_expansion())

Since now ModularFormsRing inherit from Parent you could use the test suite. You should add the following test in the constructor ModularFormsRing.__init__

TESTS::

    sage: M = ModularFormsRing(1)
    sage: TestSuite(M).run()

This test suite should also be run with other groups (especially non-congruence once) and other base rings.

The coercion is not tested, right? You should add various tests for

(modular form) + (element of ModularFormsRing)
(element of ModularFormsRing) + (modular form)
(scalar) + (element of ModularFormsRing)
(element of ModularFormsRing) + (scalar)

To determine coercions, one alternative could be to rely on what is implemented for modular forms. Namely the ModularFormsRing should have a method to obtain the modular forms of a given weight (something like M.graded_component(self, weight). It would then be possible to use coercions with respect to whatever is the result of this method.

Last edited 17 months ago by Vincent Delecroix (previous) (diff)

comment:11 Changed 18 months ago by git

Commit: 9216db9ac8697c0b49d419bdf7c49aee69c8fe41b63137012560c1a21a73009c293636f5d4c1b6b5

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

b631370updates: verifications in the constructor of GradedModularFormElement, changed some code syntax, fixed some bugs, updated documentation

comment:12 Changed 17 months ago by git

Commit: b63137012560c1a21a73009c293636f5d4c1b6b5b03643533a085889ff7fbadf97f0bc9d4e9588d5

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

b036435Implemented _richcmp_ and _bool_ for elements, fixed some typos in the documentation, updated q_expansion method

comment:13 Changed 17 months ago by git

Commit: b03643533a085889ff7fbadf97f0bc9d4e9588d571d7354f6c768f89a1d0a741306e9a6ec24ebed3

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

71d7354Merge branch 'develop' into t/31559/make_modularformrings_manipulate_formal_objects

comment:14 Changed 17 months ago by git

Commit: 71d7354f6c768f89a1d0a741306e9a6ec24ebed333de73ab1392ff625cef84d2ac9f1b43021edad3

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

33de73aimplemented _pushout_, fixed some docstrings, added doctests, fixed bugs

comment:15 Changed 17 months ago by git

Commit: 33de73ab1392ff625cef84d2ac9f1b43021edad3cfddacb40f41630b88901f7f870c43e9e4453490

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

cfddacbadded gen and ngens methods

comment:16 Changed 17 months ago by Vincent Delecroix

Looks good. Could you set to positive review needs review so that we can also have an opinion from patchbots?

is_zero would better be implemented as return not self.

Rather than self[0] == 1 I would rather use self._forms_dictionary[0].is_one() which has less indirections.

Your __getitem__ is too laxist. You could access F['a'] or F[(1,2,3)].

I am not sure we want to support rich comparison involving anything else than == or !=. What would be the meaning of F < G?

(just a remark) Technically speaking, it is conceivable to do binary operations on modular forms with different groups. It is a well defined modular forms for the intersection of the two groups (which is still finite index).

Your one element looks wrong. The dictionary should be {0: self.base_ring().one()}.

Last edited 17 months ago by Vincent Delecroix (previous) (diff)

comment:17 Changed 17 months ago by git

Commit: cfddacb40f41630b88901f7f870c43e9e4453490c5ddc445a5afff8432a688d3ccf99399c26662d7

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

c5ddc44minor updates

comment:18 Changed 17 months ago by David Ayotte

Status: newneeds_review

comment:19 Changed 17 months ago by David Ayotte

Status: needs_reviewpositive_review

comment:20 Changed 17 months ago by Vincent Delecroix

Status: positive_reviewneeds_work

comment:21 Changed 17 months ago by Vincent Delecroix

As an author, you are not supposed to switch the ticket to positive review.

comment:22 Changed 17 months ago by Vincent Delecroix

That is my fault, I meant "needs review" instead of "positive review". Sorry.

comment:23 Changed 17 months ago by David Ayotte

Status: needs_workneeds_review

No problem. I will leave it in "needs review" to see what the patchbots says.

comment:24 Changed 17 months ago by git

Commit: c5ddc445a5afff8432a688d3ccf99399c26662d790cdb170d15b4a7ecc2785977984fb12b6e580c0

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

90cdb17added operator verification for _richcmp_ method

comment:25 Changed 17 months ago by git

Commit: 90cdb170d15b4a7ecc2785977984fb12b6e580c0f58a838edcb7710a67c0c6e1a333a78bec9bcb04

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

f58a838Merge branch 'develop' into t/31559/make_modularformrings_manipulate_formal_objects

comment:26 Changed 17 months ago by David Ayotte

Status: needs_reviewneeds_work
sage: M = ModularFormsRing(Gamma1(6))
sage: f = M.0
sage: g = M.1
sage: f*g
Traceback (most recent call last):
...
ValueError: the group and/or the base ring of at least one modular form (q + 5*q^3 + 22*q^4 + 6*q^5 + O(q^6)) is not consistant with the base space

Got this error while working on #32135. This happens because, in the example above, the form f*g has trivial character hence it lives in M(Gamma0(6)).

comment:27 Changed 17 months ago by David Ayotte

After searching a bit in the code, I think that I found the origin of this error. It comes from the method __mul__ of the class ModularFormElement (in sage/modular/modform/element.py). Here's the description of the method:

def __mul__(self, other):
        r"""
        Calculate the product self * other.

        This tries to determine the
        characters of self and other, in order to avoid having to compute a
        (potentially very large) Gamma1 space. Note that this might lead to
        a modular form that is defined with respect to a larger subgroup than
        the factors are.

This is illustrated by this example:

sage: f = ModularForms(Gamma1(3), 5).0
sage: f*f
1 - 180*q^2 - 480*q^3 + 8100*q^4 + 35712*q^5 + O(q^6)
sage: (f*f).parent()
Modular Forms space of dimension 4 for Congruence Subgroup Gamma0(3) of weight 10 over Rational Field

This poses a problem as it is not always possible to add two forms with different groups:

sage: M1 = ModularForms(Gamma1(8), 8); M1
Modular Forms space of dimension 17 for Congruence Subgroup Gamma1(8) of weight 8 over Rational Field
sage: M0 = ModularForms(Gamma0(8), 8); M0
Modular Forms space of dimension 9 for Congruence Subgroup Gamma0(8) of weight 8 over Rational Field
sage: f = M1.0
sage: g = M0.0
sage: f+g
Traceback (most recent call last):
...
TypeError: unsupported operand parent(s) for +: 'Modular Forms space of dimension 17 for Congruence Subgroup Gamma1(8) of weight 8 over Rational Field' and 'Modular Forms space of dimension 9 for Congruence Subgroup Gamma0(8) of weight 8 over Rational Field'

Since the code of this ticket relies a lot on operations like these, one "easy fix" I can think of right now is to simply do not allow the creation of GradedModularFormElement for the congruence subgroups Gamma1(N) (by returning a NotImplementedError). Another fix (other ticket) would be to define some coercions between the two spaces so that it is possible to add two forms with compatible groups.

comment:28 Changed 17 months ago by Vincent Delecroix

I don't quite like the behaviour of * on modular forms. But it is not the time to change it.

One workaround would be to implement the following conversion

sage: G = Gamma1(3)
sage: M = ModularForms(Gamma1(3), 5)
sage: f = M.0
sage: M(f * f)  # should work
Traceback (most recent call last):
...
TypeError: entries must be a list of length 2

comment:29 in reply to:  28 Changed 17 months ago by David Ayotte

Replying to vdelecroix:

I don't quite like the behaviour of * on modular forms. But it is not the time to change it.

One workaround would be to implement the following conversion

sage: G = Gamma1(3)
sage: M = ModularForms(Gamma1(3), 5)
sage: f = M.0
sage: M(f * f)  # should work
Traceback (most recent call last):
...
TypeError: entries must be a list of length 2

The reason why it does not work in your example is because the form f * f is of weight 10 while the space M is of weight 5. I think that you meant to write this:

sage: f = ModularForms(Gamma1(3), 5).0
sage: M1 = ModularForms(Gamma1(3), 10)
sage: F2 = M1(f * f); F2   # this works!
1 - 180*q^2 - 480*q^3 + 8100*q^4 + 35712*q^5 + O(q^6)
sage: F2.parent()
Modular Forms space of dimension 4 for Congruence Subgroup Gamma1(3) of weight 10 over Rational Field

EDIT: the above example only works if the two spaces have the same dimension:

sage: M0 = ModularForms(Gamma0(8), 10); M0
Modular Forms space of dimension 11 for Congruence Subgroup Gamma0(8) of weight 10 over Rational Field
sage: M1 = ModularForms(Gamma1(8), 10); M1
Modular Forms space of dimension 21 for Congruence Subgroup Gamma1(8) of weight 10 over Rational Field
sage: f = M0.0
sage: M1(f)
Traceback (most recent call last):
...
TypeError: entries must be a list of length 21

However, it is possible to convert it to a form over Gamma1 by using q-expansion with enough precision:

sage: fq = f.qexp(42)
sage: M1(fq)
q + O(q^6)
Last edited 17 months ago by David Ayotte (previous) (diff)

comment:30 Changed 17 months ago by David Ayotte

Dependencies: #32168

comment:31 Changed 17 months ago by Vincent Delecroix

There are several places with trailing whitespaces (ie whitespaces before the end of line character). Usually, editors have options to remove them automatically.

comment:32 Changed 17 months ago by Vincent Delecroix

In GradedModularFormElement.__init__ the dosctring block inside the TESTS is not indented.

comment:33 Changed 17 months ago by Vincent Delecroix

In the main documentation of GradedModularFormElement there must be simple examples illustrating both input kinds

  • a dictionary {k_1:f_1, k_2:f_2, ..., k_n:f_n}
  • or a list [f_1, f_2,..., f_n]

comment:34 Changed 17 months ago by Vincent Delecroix

Here I would rather use a TypeError

            raise ValueError('the defining data structure should be a list or a dictionary')

comment:35 Changed 17 months ago by git

Commit: f58a838edcb7710a67c0c6e1a333a78bec9bcb04a6c745daa3cbe9d1717c4ac64ad434956aebf568

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

d996d40fix conversion
d54ac2afix some redundancy
2daca4eupdates
c4595c1Merge branch 'gamma1_modform_conversion' into t/31559/make_modularformrings_manipulate_formal_objects
a6c745dsmall fixes

comment:36 Changed 17 months ago by David Ayotte

Status: needs_workneeds_review

comment:37 Changed 17 months ago by git

Commit: a6c745daa3cbe9d1717c4ac64ad434956aebf568906b8ab4f2d9df7996aa26e38513d54e747e00b5

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

906b8abfixes: pyflakes, failing doctest in find_generators

comment:38 Changed 17 months ago by git

Commit: 906b8ab4f2d9df7996aa26e38513d54e747e00b5523c7b0e046249f1d0c336f9d61a733b3efbe729

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

523c7b0fix some doctring

comment:39 Changed 17 months ago by git

Commit: 523c7b0e046249f1d0c336f9d61a733b3efbe7297dbaf4eb43358fa575a5814c4e43bc7b59f37505

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

7dbaf4efix indentation error

comment:40 Changed 17 months ago by Vincent Delecroix

In the docstring of GradedModularFormElement.__init__ you wrote ``parents`` and ``forms_data`` whereas the argument names are parent, forms_datas. Moreover, I don't think you are allowed to use datas at all in english (singular is datum).

comment:41 Changed 17 months ago by Vincent Delecroix

  • In the same docstring the dictionary is quoted but not the list.
  • The class deserves a documentation. In particular the line An element of a graded ring of modular forms. should move from __init__ to the main docstring (just below the class name).
  • Currently there is no error if you ask for negative degrees : f[-1] returns 0
  • There is a remaining TODO written as a comment in the file #TODO: fix the error E4 + QQ(0). Could you either expand it or remove it?
  • Is _rmul_ really necessary? The left and right actions of scalars are the same.
  • This is a bug
    sage: M.zero().weight()
    Traceback (most recent call last):
    ...
    StopIteration: 
    
  • there is a trailing whitespace in the documentation of ModularFormSpace._pushout_

comment:42 Changed 17 months ago by git

Commit: 7dbaf4eb43358fa575a5814c4e43bc7b59f375055887973cdba9ad9b2a138fc86182c14c8d09905b

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

5887973fix docstring, changed some syntax, removed _rmul_, add negative weight checker for __getitem__, fix weight of zero element bug

comment:43 Changed 17 months ago by David Ayotte

Keywords: gsoc2021 modular forms rings added

comment:44 Changed 17 months ago by Vincent Delecroix

What about renaming find_generators.py in ring.py. It would make more sense to me. If you like it, the file find_generators.py must remain with proper deprecation warnings.

comment:45 Changed 17 months ago by Vincent Delecroix

For the string representation of the parent, I would change it to something closer to polynomials

sage: QQ['x']
Univariate Polynomial Ring in x over Rational Field

For example Ring of Modular Forms for %s over %s.

comment:46 Changed 17 months ago by Vincent Delecroix

The test suite must be run on the parent. Currently it fails

sage: M = ModularFormsRing(Gamma1(4))
sage: TestSuite(M).run()
...
The following tests failed: _test_associativity, _test_distributivity, _test_prod
Last edited 17 months ago by Vincent Delecroix (previous) (diff)

comment:47 Changed 17 months ago by Vincent Delecroix

In order to make the test suite less trivial you can also redefine .some_elements()

sage: M.some_elements()
[1 + 24*q^2 + 24*q^4 + O(q^6)]

comment:48 Changed 17 months ago by Vincent Delecroix

This is a bug (related to the failure of the test suite above)

sage: M = ModularFormsRing(Gamma1(4))
sage: a = M.gen(0)
sage: a*a
Traceback (most recent call last):
...
ValueError: the group and/or the base ring of at least one modular form (1 + 48*q^2 + 624*q^4 + O(q^6)) is not consistant with the base space

comment:49 Changed 17 months ago by Vincent Delecroix

The function def find_generators(*args): wrongly raises a NotImplementedError. Could you replace it with a proper deprecation warning so that we can remove it in some later version of sage?

comment:50 Changed 17 months ago by David Ayotte

Thank you very much Vincent for your comments. I think that moving find_generators.py in ring.py is a good idea. This means that I would move all its content in the file ring.py?

You indeed found a bug related to the conversion between two modular forms space. I have fixed it but while doing so, I found another bug related with the base ring of the modular forms ring. I'm working on this right now and hopefully I will be able to push the changes shortly.

Last edited 17 months ago by David Ayotte (previous) (diff)

comment:51 Changed 17 months ago by git

Commit: 5887973cdba9ad9b2a138fc86182c14c8d09905b7fdca220d5939f4ef9591087c8fd2d6b059516c0

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

7fdca22changed _repr_ method of ModularFormsRing, added TestSuite in doctests, added some_elements method, fixed conversion bug

comment:52 Changed 17 months ago by git

Commit: 7fdca220d5939f4ef9591087c8fd2d6b059516c01b9332ec4c3bde432908da2a2381184ad638531d

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

1b9332emoved sage.modular.modform.find_generators.py in sage.rings, attempt at making decrecation work (without success)

comment:53 in reply to:  52 Changed 17 months ago by David Ayotte

I moved the file find_generators.py in the last commit (1b9332e). I also tried to implement the deprecation framework following this guide:

https://doc.sagemath.org/html/en/developer/coding_in_python.html#deprecation.

However, I don't think it was a success because there is now one doctest that does not pass when I run the following command:

./sage -t src/sage/modular/modform/find_generators.py
File "src/sage/rings/find_generators.py", line 189, in sage.rings.find_generators.ModularFormsRing.__init__
Failed example:
    ModularFormsRing(Gamma1(13))
Expected:
    Ring of Modular Forms for Congruence Subgroup Gamma1(13) over Rational Field
Got:
    doctest:warning
    ...
    DeprecationWarning:
    Importing ModularFormsRing from here is deprecated. If you need to use it, please import it directly from sage.rings.find_generators
    See https://trac.sagemath.org/31559 for details.
    Ring of Modular Forms for Congruence Subgroup Gamma1(13) over Rational Field
**********************************************************************
1 item had failures:
   1 of  15 in sage.rings.find_generators.ModularFormsRing.__init__
    [117 tests, 1 failure, 2.24 s]
----------------------------------------------------------------------
sage -t --random-seed=0 src/sage/rings/find_generators.py  # 1 doctest failed
----------------------------------------------------------------------

I haven't figured out why this happens yet. Do you have an idea of what is the problem?

Last edited 17 months ago by David Ayotte (previous) (diff)

comment:54 Changed 16 months ago by Vincent Delecroix

The error message is rather explicit. Importing ModularFormsRing from find_generators.py is deprecated. If you want to make a doctest for this you can write something like

sage: ModularFormsRing(Gamma1(13))
doctest:warning
...
DeprecationWarning: Importing ModularFormsRing from here is deprecated...

comment:55 in reply to:  54 Changed 16 months ago by David Ayotte

Replying to vdelecroix:

The error message is rather explicit. Importing ModularFormsRing from find_generators.py is deprecated. If you want to make a doctest for this you can write something like

sage: ModularFormsRing(Gamma1(13))
doctest:warning
...
DeprecationWarning: Importing ModularFormsRing from here is deprecated...

What confuses me is that if I run the following command:

sage -t src/sage/rings/find_generators.py

I get the same error. Does this means that ModularFormsRing is imported from the old module?

comment:56 Changed 16 months ago by Vincent Delecroix

The file that contains the tests does not matter. The problem is the file that imports ModularFormsRing in the sage namespace

  • src/sage/rings/all.py

    diff --git a/src/sage/rings/all.py b/src/sage/rings/all.py
    index 50f0a4e..a2a5923 100644
    a b from .asymptotic.all import * 
    166166
    167167# Register classes in numbers abc
    168168from . import numbers_abc
     169
     170# Rings of modular forms
     171from .find_generators import ModularFormsRing

comment:57 Changed 16 months ago by Vincent Delecroix

Status: needs_reviewneeds_work

If you look at the diff, you will notice that a lot of files do not have newlines at the end.

comment:58 Changed 16 months ago by git

Commit: 1b9332ec4c3bde432908da2a2381184ad638531d68509f933d5182591ebbee5a21bea6fcd423894d

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

68509f9deprecation, added missing newline

comment:59 Changed 16 months ago by David Ayotte

Status: needs_workneeds_review

comment:60 Changed 16 months ago by Vincent Delecroix

Status: needs_reviewneeds_work

There is no need to duplicate the TestSuite three times. All these would better appear together inside an EXAMPLES or TESTS block.

+            sage: M = ModularFormsRing(1)
+            sage: TestSuite(M).run()
+            sage: M15 = ModularFormsRing(Gamma1(5))
+            sage: TestSuite(M15).run()
+            sage: M013 = ModularFormsRing(Gamma0(13))
+            sage: TestSuite(M013).run()

then

+            sage: TestSuite(ModularFormsRing(1)).run()
+            sage: TestSuite(ModularFormsRing(Gamma0(6))).run()
+            sage: TestSuite(ModularFormsRing(Gamma1(4))).run()

and finally

+            sage: TestSuite(ModularFormsRing(1)).run()
+            sage: TestSuite(ModularFormsRing(Gamma0(6))).run()
+            sage: TestSuite(ModularFormsRing(Gamma1(4))).run()

comment:61 Changed 16 months ago by Vincent Delecroix

The documentation of GradedModularFormElement is not helpful for the user The element class for ``ModularFormsRing``. You should expand a bit on what this is. Also, in this docstring it should also be advertized that the elements should not be built directly by calling the class constructor but rather via the parent.

comment:62 Changed 16 months ago by Vincent Delecroix

In the documentation of weights_list() I would emphasize that the return value for zero is the empty list. In particular, it does not correspond to what you get via weight(). Maybe this is annoying?

comment:63 Changed 16 months ago by Vincent Delecroix

Why the file find_generators.py is duplicated in both sage/rings/ and sage/modular/modform/!?

Last edited 16 months ago by Vincent Delecroix (previous) (diff)

comment:64 Changed 16 months ago by Vincent Delecroix

Also in 44 I proposed to rename find_generators.py into ring.py. You did agree in 50 to make it but this was not done.

Last edited 16 months ago by Vincent Delecroix (previous) (diff)

comment:65 Changed 16 months ago by git

Commit: 68509f933d5182591ebbee5a21bea6fcd423894d99f8ab5647e8b30a017b5fddba2fc12ad0a27988

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

2e78c45revert to commit 7fdca22
99f8ab5renamed find_generators.py to ring.py, added deprecation

comment:66 Changed 16 months ago by git

Commit: 99f8ab5647e8b30a017b5fddba2fc12ad0a2798862a9484b3e9ac98ec832266ef231b73fa2a775ff

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

62a9484docstring updates, fixed weights_list method for zero element

comment:67 Changed 16 months ago by Vincent Delecroix

There are still two files sage/modular/modform/ring.py and sage/modular/modform/find_generators.py containing the same code. What you provided is not working and is also not tested : when the import is triggered from find_generators there should be a warning.

Did you look at a file in the sage source code that implements these deprecated imports? For example sage/rings/invariant_theory.py. This is what should be done. And doctests must be added.

comment:68 Changed 16 months ago by git

Commit: 62a9484b3e9ac98ec832266ef231b73fa2a775ffed90b5a170ee62c20462979b112ac4b12625482f

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

ed90b5afixed deprecation, added deprecation for find_generators and basis_for_modform_space

comment:69 Changed 16 months ago by David Ayotte

Status: needs_workneeds_review

Hello Vincent, I hope that what I did now is correct. I followed the example you gave me. It is now much clearer in my head. Prior to this, I was looking at the guide

https://doc.sagemath.org/html/en/developer/coding_in_python.html#deprecation

and I think that some improvements could be made to make it clearer (I will probably do them later when I have time).

comment:70 Changed 16 months ago by Vincent Delecroix

Looks better. Could you move the test for the deprecation inside find_generators.py. It would be better in the latter rather than ring.py.

comment:71 Changed 16 months ago by Vincent Delecroix

Why do you put twice the same doctest

from sage.modular.modform.find_generators import find_generators
find_generators(ModularFormsRing(1))

comment:72 Changed 16 months ago by Vincent Delecroix

The documentation of a class goes in the main docstring not in the __init__ docstring. That is

class A(object):
    r"""
    This is my class A

    EXAMPLES:

    This is useful as it is the first letter of the alphabet::

        sage: a = A()
    """
    def __init__(self, a, b):
        r"""
        INPUT:
        
        - ``a`` - an a

        - ``b`` - a b
        """

comment:73 Changed 16 months ago by David Ayotte

Ok, I'll move the deprecation tests inside find_generators. It does make more sense. Considering the "same doctest", in the first one I am testing the deprecated function name (as you suggested in comment 49) and in the second one I am testing the deprecation import warning:

            sage: from sage.modular.modform.RING import find_generators
            sage: find_generators(ModularFormsRing(1))
            doctest:warning
            ...
            DeprecationWarning: find_generators is deprecated. Please use sage.modular.modform.ring.generators instead.
            See https://trac.sagemath.org/31559 for details.

        ::

            sage: from sage.modular.modform.FIND_GENERATORS import find_generators
            sage: find_generators(ModularFormsRing(1))
            doctest:warning
            ...
            DeprecationWarning: Importing find_generators from here is deprecated. If you need to use it, please import it directly from sage.modular.modform.ring
            See https://trac.sagemath.org/31559 for details.

Considering the placement of the documentation of the class, I did it like this because that's how it was done before I started working on this ticket, but I will change it, thank you for pointing it out!

comment:74 Changed 16 months ago by git

Commit: ed90b5a170ee62c20462979b112ac4b12625482fd489a1f423e066fd99bfc6ed18e5ea8a8c92a14c

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

d489a1fmoved deprecation doctests inside find_generators, fixed docstrings

comment:75 in reply to:  73 Changed 16 months ago by Vincent Delecroix

Replying to gh-DavidAyotte:

Considering the "same doctest", in the first one I am testing the deprecated function name (as you suggested in comment 49) and in the second one I am testing the deprecation import warning: <...>

Indeed. I read too quickly! Sorry.

comment:76 Changed 16 months ago by git

Commit: d489a1f423e066fd99bfc6ed18e5ea8a8c92a14cbf19eece5c801e7cbb2c170c9df6e286ccdbd506

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

bf19eecadded example info for GradedModularFormElement class, moved some doctests inside the __init__ method (for coverage)

comment:77 Changed 16 months ago by David Ayotte

In last commit, I moved some doctests inside the __init__ method of GradedModularFormElement class because it was lacking doctests.

comment:78 Changed 16 months ago by git

Commit: bf19eece5c801e7cbb2c170c9df6e286ccdbd5062e499076cadb327925414e917fa1f2c399269d2b

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

2e49907fix docbuild error

comment:79 Changed 16 months ago by Vincent Delecroix

Status: needs_reviewpositive_review

All right. Let us move forward.

comment:80 Changed 16 months ago by Matthias Köppe

Milestone: sage-9.4sage-9.5

comment:81 Changed 15 months ago by David Ayotte

Status: positive_reviewneeds_work

The index.rst file was not updated after renaming find_generator.py into ring.py and so the documentation does not show up in the reference manual. I'm fixing this.

comment:82 Changed 15 months ago by git

Commit: 2e499076cadb327925414e917fa1f2c399269d2b088139ad5f9299a129368cdb8543848ab6dcca5c

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

088139areference manual fix

comment:83 Changed 15 months ago by David Ayotte

Status: needs_workneeds_review

comment:84 Changed 15 months ago by Vincent Delecroix

Status: needs_reviewpositive_review

thanks

comment:85 Changed 15 months ago by Volker Braun

Branch: u/gh-DavidAyotte/make_modularformrings_manipulate_formal_objects088139ad5f9299a129368cdb8543848ab6dcca5c
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.