-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
coefficients
for QuasiModularFormsElement
#36269
Conversation
I am not familiar with modular forms, but wouldn't it be nice to return a (lazy) power series, analogous to
With this, it is very natural (and efficient) to get any particular coefficient. |
Thank you for letting me know. My code is actually almost a copy & paste of the same methods of sage/src/sage/modular/modform/element.py Lines 309 to 373 in 80f6d77
|
@DavidAyotte Could you check this one (or any suggestions)? |
Absolutely, I will check it in a couple days! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Seewoo, overall it looks good! I have minor docstring recommendation. Thank you for implementing this.
@mantepse Hello Martin! Yes I think it would be feasible and interesting to use the Lazy power series framework for
I will probably open a PR about this soon. |
Update docstring Co-authored-by: David Ayotte <[email protected]>
… feature/quasimod-coeff
Updated all the docstrings! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I do have more minor comments.
If X is an Integer, return coefficients for indices from 1 | ||
to X. This function caches the results of the `_compute` function. | ||
|
||
TESTS:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TESTS:: | |
EXAMPLES:: |
If you use the "TESTS" block, the doctests will not appear in the reference manual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would be still tested, right? (is TESTS::
legacy?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes both TESTS
and EXAMPLES
run the doctests, however TESTS
block does not show any examples in the Sage reference manual and I think that it is the only difference. I personally think that it is a good thing to present some examples in the manual so the user can better understand how to use the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add to my previous reply, even though it is a good practice to present some examples, sometimes some doctests aren't that useful for the user (technical or internal doctest) so you might want to hide them. But in your case the doctests are all relevant so that is why I suggested to use the EXAMPLES
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't know that examples would be also tested. Just updated!
Update documentation Co-authored-by: David Ayotte <[email protected]>
Thank you Seewoo for the changes. @mkoeppe can you approve the workflows for this PR? Thanks! |
Documentation preview for this PR (built with commit f055a04; changes) is ready! 🎉 |
Hello sorry for the delay, but I wanted to see the build jobs before setting this to postive review. There's a single doctest that does not pass and I believe it is not related with this PR, so I'm setting this to postive review. Thank you for this contribution! |
sagemathgh-36269: `coefficients` for `QuasiModularFormsElement` <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> This patch implements `_compute` and `coefficients` methods to `QuasiModularFormsElement`, which already exists in `ModularFormsElement`. I made this for my own usage. (This is my first PR for Sage, so comments are welcome!) <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [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. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36269 Reported by: Seewoo Lee Reviewer(s): David Ayotte, Seewoo Lee
This patch implements
_compute
andcoefficients
methods toQuasiModularFormsElement
, which already exists inModularFormsElement
. I made this for my own usage.(This is my first PR for Sage, so comments are welcome!)
📝 Checklist
⌛ Dependencies