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

Fix TODO in sympy/functions/special/spherical_harmonics.py #21199

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

anhnguyenphung
Copy link

@anhnguyenphung anhnguyenphung commented Mar 30, 2021

Fix 'TODO: Assert |m| <= n otherwise we should return 0' in sympy/functions/special/spherical_harmonics.py

Add the case |m| > n to return 0 in _eval_rewrite_as_polynomial() and _eval_rewrite_as_cos() of the class Ynm(Function)

Release Note

NO ENTRY

@sympy-bot
Copy link

sympy-bot commented Mar 30, 2021

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

  • No release notes entry will be added for this pull request.
Click here to see the pull request description that was parsed.
#### Fix 'TODO: Assert |m| <= n otherwise we should return 0' in sympy/functions/special/spherical_harmonics.py

#### Add the case |m| > n to return 0 in _eval_rewrite_as_polynomial() and _eval_rewrite_as_cos() of the class Ynm(Function)

#### Release Note

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

@sidhu1012
Copy link
Contributor

@anhnguyenphung It would be better to add test cases for your changes.

@anhnguyenphung
Copy link
Author

@sidhu1012 I have just added the test cases in sympy/functions/special/tests/test_spherical_harmonics.py to test my change.

@JSS95
Copy link
Collaborator

JSS95 commented Apr 14, 2021

Any more review?

@smichr
Copy link
Member

smichr commented Jun 8, 2021

We should return SymPy objects from routines like this.

@smichr
Copy link
Member

smichr commented Jun 8, 2021

If this should handle symbolic m,n then the test should be if (abs(m) - n).is_positive and a test for an indeterminate symbolic case should be added to the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants