Opened 20 months ago
Closed 15 months ago
#31559 closed enhancement (fixed)
Make ModularFormRings manipulate formal objects
Reported by:  Vincent Delecroix  Owned by:  David Ayotte 

Priority:  major  Milestone:  sage9.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: 
Description (last modified by )
Currently, ModularFormRings
is manipulating truncated qseries. 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
Milestone:  sage9.3 → sage9.4 

comment:2 Changed 18 months ago by
Description:  modified (diff) 

comment:3 Changed 18 months ago by
Branch:  → u/ghDavidAyotte/make_modularformrings_manipulate_formal_objects 

comment:4 Changed 18 months ago by
Commit:  → 5f23e4db95190358b0203af006ca36b0e9973b25 

Owner:  set to David Ayotte 
New commits:
5f23e4d  Initial implementation of GradedModularFormElement. The class ModularFormsRing now inherit from Parent and UniqueRepresentation

comment:5 Changed 18 months ago by
Commit:  5f23e4db95190358b0203af006ca36b0e9973b25 → 33a8cd5deb9ae0a133461af2301044ceb3f01ab6 

Branch pushed to git repo; I updated commit sha1. New commits:
33a8cd5  Added the methods _neq_ and is_homogeneous.

comment:6 Changed 18 months ago by
Commit:  33a8cd5deb9ae0a133461af2301044ceb3f01ab6 → 6faa4baaccaf4f6dd5f6fc9b59aa29706f8d8c0b 

Branch pushed to git repo; I updated commit sha1. New commits:
6faa4ba  Implemented coercion. Added documentation. Fixed a bug in _add_ method.

comment:7 Changed 18 months ago by
Commit:  6faa4baaccaf4f6dd5f6fc9b59aa29706f8d8c0b → 9216db9ac8697c0b49d419bdf7c49aee69c8fe41 

Branch pushed to git repo; I updated commit sha1. New commits:
9216db9  Moved the class GradedModularFormElement into modular/modform/element.py. Fixed some issues with coercion.

comment:8 Changed 18 months ago by
Authors:  → David Ayotte 

Reviewers:  → Vincent Delecroix 
comment:9 Changed 18 months ago by
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
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 noncongruence 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.
comment:11 Changed 18 months ago by
Commit:  9216db9ac8697c0b49d419bdf7c49aee69c8fe41 → b63137012560c1a21a73009c293636f5d4c1b6b5 

Branch pushed to git repo; I updated commit sha1. New commits:
b631370  updates: verifications in the constructor of GradedModularFormElement, changed some code syntax, fixed some bugs, updated documentation

comment:12 Changed 17 months ago by
Commit:  b63137012560c1a21a73009c293636f5d4c1b6b5 → b03643533a085889ff7fbadf97f0bc9d4e9588d5 

Branch pushed to git repo; I updated commit sha1. New commits:
b036435  Implemented _richcmp_ and _bool_ for elements, fixed some typos in the documentation, updated q_expansion method

comment:13 Changed 17 months ago by
Commit:  b03643533a085889ff7fbadf97f0bc9d4e9588d5 → 71d7354f6c768f89a1d0a741306e9a6ec24ebed3 

Branch pushed to git repo; I updated commit sha1. New commits:
71d7354  Merge branch 'develop' into t/31559/make_modularformrings_manipulate_formal_objects

comment:14 Changed 17 months ago by
Commit:  71d7354f6c768f89a1d0a741306e9a6ec24ebed3 → 33de73ab1392ff625cef84d2ac9f1b43021edad3 

Branch pushed to git repo; I updated commit sha1. New commits:
33de73a  implemented _pushout_, fixed some docstrings, added doctests, fixed bugs

comment:15 Changed 17 months ago by
Commit:  33de73ab1392ff625cef84d2ac9f1b43021edad3 → cfddacb40f41630b88901f7f870c43e9e4453490 

Branch pushed to git repo; I updated commit sha1. New commits:
cfddacb  added gen and ngens methods

comment:16 Changed 17 months ago by
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()}
.
comment:17 Changed 17 months ago by
Commit:  cfddacb40f41630b88901f7f870c43e9e4453490 → c5ddc445a5afff8432a688d3ccf99399c26662d7 

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

comment:18 Changed 17 months ago by
Status:  new → needs_review 

comment:19 Changed 17 months ago by
Status:  needs_review → positive_review 

comment:20 Changed 17 months ago by
Status:  positive_review → needs_work 

comment:21 Changed 17 months ago by
As an author, you are not supposed to switch the ticket to positive review.
comment:22 Changed 17 months ago by
That is my fault, I meant "needs review" instead of "positive review". Sorry.
comment:23 Changed 17 months ago by
Status:  needs_work → needs_review 

No problem. I will leave it in "needs review" to see what the patchbots says.
comment:24 Changed 17 months ago by
Commit:  c5ddc445a5afff8432a688d3ccf99399c26662d7 → 90cdb170d15b4a7ecc2785977984fb12b6e580c0 

Branch pushed to git repo; I updated commit sha1. New commits:
90cdb17  added operator verification for _richcmp_ method

comment:25 Changed 17 months ago by
Commit:  90cdb170d15b4a7ecc2785977984fb12b6e580c0 → f58a838edcb7710a67c0c6e1a333a78bec9bcb04 

Branch pushed to git repo; I updated commit sha1. New commits:
f58a838  Merge branch 'develop' into t/31559/make_modularformrings_manipulate_formal_objects

comment:26 Changed 17 months ago by
Status:  needs_review → needs_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
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 followup: 29 Changed 17 months ago by
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 Changed 17 months ago by
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 qexpansion with enough precision:
sage: fq = f.qexp(42) sage: M1(fq) q + O(q^6)
comment:30 Changed 17 months ago by
Dependencies:  → #32168 

comment:31 Changed 17 months ago by
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
In GradedModularFormElement.__init__
the dosctring block inside the TESTS
is not indented.
comment:33 Changed 17 months ago by
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
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
Commit:  f58a838edcb7710a67c0c6e1a333a78bec9bcb04 → a6c745daa3cbe9d1717c4ac64ad434956aebf568 

comment:36 Changed 17 months ago by
Status:  needs_work → needs_review 

comment:37 Changed 17 months ago by
Commit:  a6c745daa3cbe9d1717c4ac64ad434956aebf568 → 906b8ab4f2d9df7996aa26e38513d54e747e00b5 

Branch pushed to git repo; I updated commit sha1. New commits:
906b8ab  fixes: pyflakes, failing doctest in find_generators

comment:38 Changed 17 months ago by
Commit:  906b8ab4f2d9df7996aa26e38513d54e747e00b5 → 523c7b0e046249f1d0c336f9d61a733b3efbe729 

Branch pushed to git repo; I updated commit sha1. New commits:
523c7b0  fix some doctring

comment:39 Changed 17 months ago by
Commit:  523c7b0e046249f1d0c336f9d61a733b3efbe729 → 7dbaf4eb43358fa575a5814c4e43bc7b59f37505 

Branch pushed to git repo; I updated commit sha1. New commits:
7dbaf4e  fix indentation error

comment:40 Changed 17 months ago by
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
 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]
returns0
 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
Commit:  7dbaf4eb43358fa575a5814c4e43bc7b59f37505 → 5887973cdba9ad9b2a138fc86182c14c8d09905b 

Branch pushed to git repo; I updated commit sha1. New commits:
5887973  fix 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
Keywords:  gsoc2021 modular forms rings added 

comment:44 Changed 17 months ago by
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
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
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
comment:47 Changed 17 months ago by
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
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
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
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.
comment:51 Changed 17 months ago by
Commit:  5887973cdba9ad9b2a138fc86182c14c8d09905b → 7fdca220d5939f4ef9591087c8fd2d6b059516c0 

Branch pushed to git repo; I updated commit sha1. New commits:
7fdca22  changed _repr_ method of ModularFormsRing, added TestSuite in doctests, added some_elements method, fixed conversion bug

comment:52 followup: 53 Changed 17 months ago by
Commit:  7fdca220d5939f4ef9591087c8fd2d6b059516c0 → 1b9332ec4c3bde432908da2a2381184ad638531d 

Branch pushed to git repo; I updated commit sha1. New commits:
1b9332e  moved sage.modular.modform.find_generators.py in sage.rings, attempt at making decrecation work (without success)

comment:53 Changed 17 months ago by
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 randomseed=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?
comment:54 followup: 55 Changed 16 months ago by
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 Changed 16 months ago by
Replying to vdelecroix:
The error message is rather explicit. Importing
ModularFormsRing
fromfind_generators.py
is deprecated. If you want to make a doctest for this you can write something likesage: 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
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 * 166 166 167 167 # Register classes in numbers abc 168 168 from . import numbers_abc 169 170 # Rings of modular forms 171 from .find_generators import ModularFormsRing
comment:57 Changed 16 months ago by
Status:  needs_review → needs_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
Commit:  1b9332ec4c3bde432908da2a2381184ad638531d → 68509f933d5182591ebbee5a21bea6fcd423894d 

Branch pushed to git repo; I updated commit sha1. New commits:
68509f9  deprecation, added missing newline

comment:59 Changed 16 months ago by
Status:  needs_work → needs_review 

comment:60 Changed 16 months ago by
Status:  needs_review → needs_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
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
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
Why the file find_generators.py
is duplicated in both sage/rings/
and sage/modular/modform/
!?
comment:64 Changed 16 months ago by
comment:65 Changed 16 months ago by
Commit:  68509f933d5182591ebbee5a21bea6fcd423894d → 99f8ab5647e8b30a017b5fddba2fc12ad0a27988 

comment:66 Changed 16 months ago by
Commit:  99f8ab5647e8b30a017b5fddba2fc12ad0a27988 → 62a9484b3e9ac98ec832266ef231b73fa2a775ff 

Branch pushed to git repo; I updated commit sha1. New commits:
62a9484  docstring updates, fixed weights_list method for zero element

comment:67 Changed 16 months ago by
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
Commit:  62a9484b3e9ac98ec832266ef231b73fa2a775ff → ed90b5a170ee62c20462979b112ac4b12625482f 

Branch pushed to git repo; I updated commit sha1. New commits:
ed90b5a  fixed deprecation, added deprecation for find_generators and basis_for_modform_space

comment:69 Changed 16 months ago by
Status:  needs_work → needs_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
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
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
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 followup: 75 Changed 16 months ago by
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
Commit:  ed90b5a170ee62c20462979b112ac4b12625482f → d489a1f423e066fd99bfc6ed18e5ea8a8c92a14c 

Branch pushed to git repo; I updated commit sha1. New commits:
d489a1f  moved deprecation doctests inside find_generators, fixed docstrings

comment:75 Changed 16 months ago by
Replying to ghDavidAyotte:
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
Commit:  d489a1f423e066fd99bfc6ed18e5ea8a8c92a14c → bf19eece5c801e7cbb2c170c9df6e286ccdbd506 

Branch pushed to git repo; I updated commit sha1. New commits:
bf19eec  added example info for GradedModularFormElement class, moved some doctests inside the __init__ method (for coverage)

comment:77 Changed 16 months ago by
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
Commit:  bf19eece5c801e7cbb2c170c9df6e286ccdbd506 → 2e499076cadb327925414e917fa1f2c399269d2b 

Branch pushed to git repo; I updated commit sha1. New commits:
2e49907  fix docbuild error

comment:79 Changed 16 months ago by
Status:  needs_review → positive_review 

All right. Let us move forward.
comment:80 Changed 16 months ago by
Milestone:  sage9.4 → sage9.5 

comment:81 Changed 15 months ago by
Status:  positive_review → needs_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
Commit:  2e499076cadb327925414e917fa1f2c399269d2b → 088139ad5f9299a129368cdb8543848ab6dcca5c 

Branch pushed to git repo; I updated commit sha1. New commits:
088139a  reference manual fix

comment:83 Changed 15 months ago by
Status:  needs_work → needs_review 

comment:85 Changed 15 months ago by
Branch:  u/ghDavidAyotte/make_modularformrings_manipulate_formal_objects → 088139ad5f9299a129368cdb8543848ab6dcca5c 

Resolution:  → fixed 
Status:  positive_review → closed 
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.