Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-homogeneous GradedModularFormElement multiplication and exponentiation is incorrect #35932

Closed
2 tasks done
grhkm21 opened this issue Jul 11, 2023 · 4 comments · Fixed by #35978 or #35944
Closed
2 tasks done

Comments

@grhkm21
Copy link
Contributor

grhkm21 commented Jul 11, 2023

Steps To Reproduce

Code:

sage: M = ModularFormsRing(1)
sage: E4, E6 = M.gens()
sage: E4 + M(1)
2 + 240*q + 2160*q^2 + 6720*q^3 + 17520*q^4 + 30240*q^5 + O(q^6)
sage: (E4 + M(1))^2
3 + 720*q + 64080*q^2 + 1056960*q^3 + 7943760*q^4 + 37530720*q^5 + O(q^6)

Expected Behavior

Well, it should start with 4 + ... since E4 + M(1) start with 2 + .... In particular, it should be E4^2 + 2 * E4 + 1 instead of the current computed E4^2 + E4 + 1.

Actual Behavior

I found the buggy line, so I will just report it (and make a PR soon): On line 3558 of sage.modular.modform, i.e. inside the _mul_ method of GradedModularFormElement, it has the following incorrect line:

        GM = self.__class__
        f_self = self._forms_dictionary
        f_other = other._forms_dictionary
        f_mul = {}
        for k_self in f_self.keys():
            for k_other in f_other.keys():
                f_mul[k_self + k_other] = f_self[k_self]*f_other[k_other]        # Line 3558
        return GM(self.parent(), f_mul)

It should be += instead of =, just like normal polynomial multiplication.

Additional Information

No response

Environment

- **Sage Version**: 10.0

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
@grhkm21 grhkm21 added the t: bug label Jul 11, 2023
@grhkm21
Copy link
Contributor Author

grhkm21 commented Jul 11, 2023

Is it possible for me to add labels (modular-forms) myself? Or can I not because I haven't made any contributions to the repo before?

@sheerluck
Copy link
Contributor

--- a/element.py
+++ b/element.py
@@ -3555,7 +3555,10 @@
         f_mul = {}
         for k_self in f_self.keys():
             for k_other in f_other.keys():
-                f_mul[k_self + k_other] = f_self[k_self]*f_other[k_other]
+                if k_self + k_other in f_mul:
+                    f_mul[k_self + k_other] += f_self[k_self]*f_other[k_other]
+                else:
+                    f_mul[k_self + k_other] = f_self[k_self]*f_other[k_other]
         return GM(self.parent(), f_mul)
 
     def _lmul_(self, c):

with that I get 4 + 960*q + 66240*q^2 + 1063680*q^3 + 7961280*q^4 + 37560960*q^5 + O(q^6) for (E4 + M(1)) ^ 2

@grhkm21
Copy link
Contributor Author

grhkm21 commented Jul 14, 2023

@sheerluck Oops, I forgot to push the patch. I will push it to #35944 since it's relevant for that as well

Screenshot 2023-07-14 at 16 12 07

@DavidAyotte
Copy link
Member

Hello, this is indeed a bug, thank you for reporting this and fixing it. I will review the patch once it is finished.

vbraun pushed a commit to vbraun/sage that referenced this issue Oct 15, 2023
sagemathgh-35978: Fix GradedModularFormElement multiplication
    
Fix sagemath#35932 - incorrect multiplication for GradedModularFormElement.

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#35978
Reported by: grhkm21
Reviewer(s): David Ayotte, grhkm21, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this issue Oct 17, 2023
sagemathgh-35978: Fix GradedModularFormElement multiplication
    
Fix sagemath#35932 - incorrect multiplication for GradedModularFormElement.

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
    
URL: sagemath#35978
Reported by: grhkm21
Reviewer(s): David Ayotte, grhkm21, Matthias Köppe
@vbraun vbraun closed this as completed in 7a92489 Oct 21, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 21, 2023
vbraun pushed a commit to vbraun/sage that referenced this issue Feb 10, 2025
sagemathgh-35944: Fix incorrect exponentiation of modular forms
    
Fixes sagemath#35932 incorrect exponentation of modular forms.

~~This is also my first PR, please provide suggestions on any mistakes I
made.~~ Not anymore lol
    
URL: sagemath#35944
Reported by: grhkm21
Reviewer(s): David Ayotte, grhkm21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants