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 divergence in cylindrical coordinates #20047
Conversation
✅ Hi, I am the SymPy bot (v162). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.10. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
Looks good. Please add test cases. |
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.
@lindsayad please fix the flake test. Importing BaseScalar
is not needed. I think then we can get this merged.
This fix is more general than my particular use case, but I wanted to compute: ```python from sympy.vector import divergence, gradient, Vector, CoordSys3D R = CoordSys3D('R', transformation='cylindrical') divergence(R.i) ``` The correct solution to this is 1/r, however, sympy returned zero because it was detecting that no base scalars were in the provided expression, and then using the special case code, returning zero. A straight-forward solution to this is removing the special case code, and simply returning a `Derivative` instantiation every time.
5f532d2
to
a755a67
Compare
@lindsayad Would you like to conclude this PR? Please let us know and do NOT close this PR. Thanks for your contributions. |
I would like to see it concluded. It looks like my change broke this test:
I'm not surprised by this. I guess from my perspective, I would rather have more verbose output than sometimes have wrong output. If you guys agree, then I will change that test. I'm also open to other suggestions. |
This notation seems better to me. At least it makes clear that @moorepants Any thoughts on this? |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[907895ac] [87cbdd9a]
+ 7.47±0.3ms 12.1±0.8ms 1.62 matrices.TimeMatrixPower.time_Case1
Full benchmark results can be found as artifacts in GitHub Actions |
I will open an issue for adding a test. |
Just yesterday I was on a new machine and manually updated this file when doing some MMS testing and I thought to myself that I really need to get this in. Thanks for getting this over the finish line! |
This fix is more general than my particular use case, but I wanted to
compute:
The correct solution to this is 1/r, however, sympy returned zero
because it was detecting that no base scalars were in the provided
expression, and then using the special case code, returning zero. A
straight-forward solution to this is removing the special case code, and
simply returning a
Derivative
instantiation every time.Derivative
instance from_diff_conditional
inoperators.py
since thecoeffs
may be functions of the base scalars even ifexpr
is not