Opened 6 years ago
Last modified 4 months ago
#17909 needs_work enhancement
Rankin Cohen brackets for (quasi) modular forms for Hecke triangle groups
Reported by:  jj  Owned by:  

Priority:  minor  Milestone:  sage9.4 
Component:  modular forms  Keywords:  modular forms hecke triangle groups rankin cohen bracket 
Cc:  mraum, vdelecroix, cremona  Merged in:  
Authors:  Jonas Jermann  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/jj/rankin_cohen_bracket (Commits, GitHub, GitLab)  Commit:  2de1a1b54ff7bfde193f41fd26054d6126ca2cc1 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket adds support for Rankin Cohen brackets for (quasi) modular forms:
 http://arxiv.org/abs/math/0509653
 http://arxiv.org/abs/1306.3634v2
 http://math.univbpclermont.fr/~royer/ens/Mali/M2_Mali.html
The ticket also adds support for listing the homogeneous and quasi parts of an element and fixes some bugs.
Change History (53)
comment:1 Changed 6 years ago by
 Commit changed from bf892415fb7fd43df17cd4c50f422c2212f7f0cc to fd1edffd315afd3f706af102f4637314485d4595
comment:2 Changed 6 years ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:3 Changed 6 years ago by
 Description modified (diff)
comment:4 Changed 6 years ago by
 Commit changed from fd1edffd315afd3f706af102f4637314485d4595 to 38dfd7ac983fad06590058230388afeaa0dac011
comment:5 Changed 6 years ago by
 Commit changed from 38dfd7ac983fad06590058230388afeaa0dac011 to ca28b8ba55b3c69ac03987a9f0cf0de180679fbf
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
987392f  allow coercions from the forms over n=3 to forms over n=infinity

a91d126  add the multiplicationbyweight operator

2b6826a  initial version of Rankin Cohen brackets

ca28b8b  add missing doctests and documentation

comment:6 Changed 6 years ago by
 Commit changed from ca28b8ba55b3c69ac03987a9f0cf0de180679fbf to 13968052652e3c480ab9025a827ad5eb986ed177
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
48d9918  allow coercions from the forms over n=3 to forms over n=infinity

21b72f7  add the multiplicationbyweight operator

781a7b2  initial version of Rankin Cohen brackets

1396805  add missing doctests and documentation

comment:7 Changed 6 years ago by
 Milestone changed from sage6.6 to sage6.7
Rebase on u/jj/theta_coercion
comment:8 Changed 6 years ago by
 Description modified (diff)
comment:9 Changed 6 years ago by
 Commit changed from 13968052652e3c480ab9025a827ad5eb986ed177 to 47d19029802f162fedf233c0a8a0e9883ca94d70
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7d9146a  allow coercions from the forms over n=3 to forms over n=infinity

4d36e7a  trac #17261 fixing doc formatting in readme

dbf5f26  trac #17261 again better doc in readme

ba88e5e  improvements after review from chapoton

e2a4ef8  add the multiplicationbyweight operator

be0e7ad  initial version of Rankin Cohen brackets

47d1902  add missing doctests and documentation

comment:10 Changed 6 years ago by
 Milestone changed from sage6.7 to sage6.10
comment:11 Changed 6 years ago by
 Commit changed from 47d19029802f162fedf233c0a8a0e9883ca94d70 to 73de05346e640f7e6cdb34bdabcabec76b775af0
comment:12 Changed 5 years ago by
 Commit changed from 73de05346e640f7e6cdb34bdabcabec76b775af0 to 2fd8e13d01840c9902020cc1baa864b844868b91
comment:13 Changed 5 years ago by
 Milestone changed from sage6.10 to sage7.0
comment:14 Changed 5 years ago by
 Priority changed from minor to major
comment:15 Changed 5 years ago by
 Branch changed from u/jj/rankin_cohen_bracket to u/chapoton/17909
 Commit changed from 2fd8e13d01840c9902020cc1baa864b844868b91 to fb03d29e2609f0d406fb35b7430c6b6e5159286c
 Milestone changed from sage7.0 to sage7.4
comment:17 Changed 4 years ago by
 Branch changed from u/chapoton/17909 to u/jj/rankin_cohen_bracket
 Commit changed from fb03d29e2609f0d406fb35b7430c6b6e5159286c to bef5a6b5d1c8811ea0ac0ed2fa4163bc294bfa4d
 Dependencies #17261 deleted
 Priority changed from major to minor
comment:18 Changed 4 years ago by
 Commit changed from bef5a6b5d1c8811ea0ac0ed2fa4163bc294bfa4d to 9b46edd8f387eec7a35376217787f4d174a1fa11
Branch pushed to git repo; I updated commit sha1. New commits:
9b46edd  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

comment:19 Changed 4 years ago by
 Commit changed from 9b46edd8f387eec7a35376217787f4d174a1fa11 to 85bdbfde6d571f20eff94ed4faf6d10401d6a353
Branch pushed to git repo; I updated commit sha1. New commits:
85bdbfd  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

comment:20 Changed 4 years ago by
 Description modified (diff)
comment:21 Changed 4 years ago by
comment:22 Changed 4 years ago by
 Commit changed from 85bdbfde6d571f20eff94ed4faf6d10401d6a353 to 16c5d1da187fb25851354bfc7f14d8786eeeee14
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
439e598  add the multiplicationbyweight operator

b180c56  initial version of Rankin Cohen brackets

c50b4ce  add missing doctests and documentation

7a47fa1  apply modern import commit from chapoton

f15ab00  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

16c5d1d  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

comment:23 Changed 4 years ago by
 Commit changed from 16c5d1da187fb25851354bfc7f14d8786eeeee14 to 687e9a0962171d24a9ed12516c4989cd09a63c8d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b5b8ef5  add the multiplicationbyweight operator

56a3642  initial version of Rankin Cohen brackets

c7ac17e  add missing doctests and documentation

fb4b2f5  apply modern import commit from chapoton

3856464  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

687e9a0  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

comment:24 Changed 4 years ago by
 Commit changed from 687e9a0962171d24a9ed12516c4989cd09a63c8d to f616890e684f22208132417996e7d8263e3af10e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
08957bf  add the multiplicationbyweight operator

bd26578  initial version of Rankin Cohen brackets

38d092b  add missing doctests and documentation

7d46d36  apply modern import commit from chapoton

4adea7d  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

f616890  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

comment:25 Changed 4 years ago by
 Commit changed from f616890e684f22208132417996e7d8263e3af10e to ffe93d8ff4f71738fd9bec0217cd91cc2c00bf1e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b410a00  add the multiplicationbyweight operator

cccd1c4  initial version of Rankin Cohen brackets

fb7ad68  add missing doctests and documentation

4eb7759  apply modern import commit from chapoton

6eb4bd4  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

ffe93d8  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

comment:26 Changed 4 years ago by
 Milestone changed from sage8.0 to sage8.2
comment:27 Changed 3 years ago by
 Commit changed from ffe93d8ff4f71738fd9bec0217cd91cc2c00bf1e to c23aac2f3fd09feb73080fc0a8ef13c3bf602cdd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5b1a331  add the multiplicationbyweight operator

21b7149  initial version of Rankin Cohen brackets

748f0b5  add missing doctests and documentation

6b3155f  apply modern import commit from chapoton

21743d0  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

c23aac2  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

comment:28 Changed 3 years ago by
 Milestone changed from sage8.2 to sage8.5
comment:29 Changed 3 years ago by
 fix the pyflakes warning (see patchbot plugin)
 use the
:arxiv:`math/0509653v2`
syntax for the links
 There should be a double colon here:
+ EXAMPLES: +
 this
+ if len(laurent_series.exponents()) == 0:
could be just
+ if not laurent_series.exponents():
 I do not like the change from "Basis matrix:" to "User basis matrix:"
 Here you should insert a blank line between the 2 lines
+ Return [self,g]_m, the ``m``th Rankin Cohen bracket of ``self`` with ``g``. + See ``self.parent().rankin_cohen_bracket`` for more information.
same here:
+ Return the summands of ``self`` divided by their depths. + This assumes that ``self`` has a depth (see :meth:`has_depth`).
 Avoid using
m=ZZ(1)
inside the function arguments. Use m=None there, and set m to 1 inside the function, if m is None.
comment:30 Changed 3 years ago by
 Commit changed from c23aac2f3fd09feb73080fc0a8ef13c3bf602cdd to 7482a7e21ecf6f289e568f9c5d121b04352d87dc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
eca2bb4  add the multiplicationbyweight operator

5e1290b  initial version of Rankin Cohen brackets

52d8357  add missing doctests and documentation

ac9dd9b  apply modern import commit from chapoton

908ba60  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

6eab142  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

7482a7e  adjustments after review

comment:31 Changed 3 years ago by
hi chapoton
Thanks for the review. I tried to fix as much as possible.
Regarding the "User basis matrix": That's due to the change "ambient_space._module.submodule" > "ambient_space._module.submodule_with_basis". This is from like 4 years ago but I remember having had a very good reason for the change (it's a bugfix which I included in this change). If I recall correctly it just makes more sense to have coordinates with respect to the given basis. Why don't you like the change?
I executed pyflakes modform_hecketriangle/*.py and tried to fix most, what should I put in all.py and init.py?
Regards Jonas
comment:32 Changed 3 years ago by
ok, thanks for the explanation about User basis matrix. I am no longer opposed to this change.
Concerning pyflakes, there is nothing to fix in all.py and init.py. I was only refering to the pyflakes plugin (that you can see in some but not all of the patchbot reports): it was complaining about
+src/sage/modular/modform_hecketriangle/abstract_space.py:87: local variable 'numerator_parent' is assigned to but never used
Did you fix that ?
comment:33 Changed 3 years ago by
Yes, I removed all other warnings.
comment:34 Changed 3 years ago by
 Commit changed from 7482a7e21ecf6f289e568f9c5d121b04352d87dc to d5c281377e000c7d820d9895fda587f8f29c0204
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ca0f922  add the multiplicationbyweight operator

29dba33  initial version of Rankin Cohen brackets

59d94e7  add missing doctests and documentation

c2fb419  apply modern import commit from chapoton

8aaac49  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

71f5240  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

d5c2813  adjustments after review

comment:35 Changed 3 years ago by
 Commit changed from d5c281377e000c7d820d9895fda587f8f29c0204 to 85c03e3a502cca32fd28bf9471fd09dfc55a9773
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
59e9bd8  add the multiplicationbyweight operator

fde5a73  initial version of Rankin Cohen brackets

714307e  add missing doctests and documentation

7c4ff2e  apply modern import commit from chapoton

3692cbd  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

173ffb2  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

85c03e3  adjustments after review

comment:36 Changed 2 years ago by
 Commit changed from 85c03e3a502cca32fd28bf9471fd09dfc55a9773 to 884e7a9ff4839e50bb488b5812bdd3ea4992f0ba
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
51de4fa  add the multiplicationbyweight operator

1811688  initial version of Rankin Cohen brackets

443e5f1  add missing doctests and documentation

4355e1f  apply modern import commit from chapoton

a9158e5  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

3547b2f  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

884e7a9  adjustments after review

comment:37 Changed 2 years ago by
 Milestone changed from sage8.5 to sage8.7
comment:38 Changed 2 years ago by
 Milestone changed from sage8.7 to sage8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:39 Changed 2 years ago by
 Commit changed from 884e7a9ff4839e50bb488b5812bdd3ea4992f0ba to b704800e4d11fed54a48f15e3d331c72f6f9b029
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1c32a9c  add the multiplicationbyweight operator

19cc17f  initial version of Rankin Cohen brackets

7f0994e  add missing doctests and documentation

af364ff  apply modern import commit from chapoton

4bb002a  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

6d21480  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

b704800  adjustments after review

comment:40 Changed 2 years ago by
 Commit changed from b704800e4d11fed54a48f15e3d331c72f6f9b029 to 38a56abab3ce9e910825c81201807179c477887a
Branch pushed to git repo; I updated commit sha1. New commits:
38a56ab  import reduce from functools

comment:41 Changed 2 years ago by
 Commit changed from 38a56abab3ce9e910825c81201807179c477887a to 40e4b949bdd12bd9c446445d4314b7ae8f58d812
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
93ff788  add the multiplicationbyweight operator

b626128  initial version of Rankin Cohen brackets

4405776  add missing doctests and documentation

6815521  apply modern import commit from chapoton

d206d0f  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

b69818c  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

65ecd1b  adjustments after review

40e4b94  import reduce from functools

comment:42 Changed 2 years ago by
 Description modified (diff)
 Milestone changed from sage8.8 to sage8.9
comment:43 Changed 2 years ago by
 Commit changed from 40e4b949bdd12bd9c446445d4314b7ae8f58d812 to 2de1a1b54ff7bfde193f41fd26054d6126ca2cc1
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6c33132  add the multiplicationbyweight operator

85c3632  initial version of Rankin Cohen brackets

69d5929  add missing doctests and documentation

68d07d6  apply modern import commit from chapoton

6cb5754  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

48a06c1  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

86c37a8  adjustments after review

2de1a1b  import reduce from functools

comment:44 Changed 2 years ago by
 Description modified (diff)
New commits:
6c33132  add the multiplicationbyweight operator

85c3632  initial version of Rankin Cohen brackets

69d5929  add missing doctests and documentation

68d07d6  apply modern import commit from chapoton

6cb5754  use submodule_with_basis instead of submodule, so that coordinate vectors really correspond to the specified basis, bugfix regarding exception

48a06c1  make series interpretation work more generally with basic validation, allow trivial series in rationalize_series, don't forget denom_factor

86c37a8  adjustments after review

2de1a1b  import reduce from functools

comment:45 Changed 2 years ago by
The ticket is in status needs review since 4 years. Will it never be reviewed/merged?
comment:46 Changed 18 months ago by
 Milestone changed from sage8.9 to sage9.1
Ticket retargeted after milestone closed
comment:47 Changed 14 months ago by
 Milestone changed from sage9.1 to sage9.2
Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.
comment:49 Changed 12 months ago by
Why did this change to needs_work? It was green the last time I worked on it and it was ready for review since over 5 years (it keeps breaking over time for other changes in sage and I have to keep fixing it :(). Also I somehow don't find my branch anymore??
comment:50 Changed 12 months ago by
 Cc cremona added
Hello,
I set the ticket to "needs works" because the branch turned red, which indicates a merge conflict with the latest beta release.
I can hear and understand your frustration. Because we are so few active people, it is very difficult for authors of tickets to get their code reviewed. And even more because of the mathematical nature of the tickets, that requires some mathematical understanding from the reviewer. It seems that most of the people that were very active on modular forms at the beginning of sage have left the boat, and turned to other interests.
Maybe you could contact people active with lmfdb.org and ask them if they could find somebody (some expert) to review your ticket ? I have added John Cremona in cc here ; maybe he can help somehow.
comment:51 Changed 12 months ago by
Oh wow, that was a fast answer. I am no longer in academics since ~5 years, I now work as a software engineer. So I don't need this to be merged. I just wanted to provide the work to sage / not to just get lost. If someone is still interested to have it merged I can fix it again. It seems the way how to handle git has changed. What do I do to checkout my branch + fix/push it?
comment:52 Changed 9 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:53 Changed 4 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
Branch pushed to git repo; I updated commit sha1. New commits:
add missing doctests and documentation