lambdify loggamma now works for mpmath #19913

abhaydhiman · 2020-08-07T05:06:19Z

References to other Issues or PRs

Brief description of what is fixed or changed

Lambdifying an expression with loggamma doesn't work for mpmath previously but I made some fixes so that it works. File changed are pycode and test_pycode.

Other comments

Nan

Release Notes

printing
- The mpmath code printer now correctly prints the loggamma function.
utilities
- Lambdifying an expression with loggamma using mpmath no longer raises ImportError.

sympy-bot · 2020-08-07T05:06:28Z

✅

Hi, I am the SymPy bot (v160). 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:

printing
- The mpmath code printer now correctly prints the loggamma function. (#19913 by @abhaydhiman)
utilities
- Lambdifying an expression with loggamma using mpmath no longer raises ImportError. (#19913 by @abhaydhiman)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixed #17411

#### Brief description of what is fixed or changed
Lambdifying an expression with loggamma doesn't work for mpmath previously but I made some fixes so that it works. File changed are pycode and test_pycode.

#### Other comments
Nan

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* printing
    * The mpmath code printer now correctly prints the `loggamma` function.
* utilities
    * Lambdifying an expression with `loggamma` using mpmath no longer raises `ImportError`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

codecov · 2020-08-07T08:53:49Z

Codecov Report

Merging #19913 into master will decrease coverage by 4.467%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##            master    #19913       +/-   ##
=============================================
- Coverage   75.811%   71.344%   -4.468%     
=============================================
  Files          668       668               
  Lines       173237    173186       -51     
  Branches     40841     40842        +1     
=============================================
- Hits        131334    123558     -7776     
- Misses       36172     43777     +7605     
- Partials      5731      5851      +120

sympy/utilities/tests/test_lambdify.py

abhaydhiman · 2020-08-07T11:26:53Z

As sympy.__dict__ doesn't contains lgamma. Its used to pass the test case for lgamma as main function for it is defined in mpmath.__dict__.

sympy/utilities/lambdify.py

eric-wieser · 2020-08-09T11:31:01Z

You need to change the mpmath specific lines I linked to above.

abhaydhiman · 2020-08-09T11:33:58Z

You need to change the mpmath specific lines I linked to above.

What exactly do I have to change in that line of code.

eric-wieser · 2020-08-09T11:39:40Z

Updating the dictionary to correct the lgamma key on the next line would probably do the trick.

abhaydhiman · 2020-08-09T11:44:18Z

well, I actually did that.

sympy/printing/pycode.py

eric-wieser · 2020-08-09T11:50:28Z

You should add to _known_functions_mpmath

abhaydhiman · 2020-08-09T12:52:18Z

'lgamma': 'loggamma' if I add this in _known_functions_mpmath that again gives the same error. Or I have to add "loggamma": "loggamma" without deleting the previous "loggamma": "lgamma", I tried this and this works perfectly fine.

eric-wieser · 2020-08-09T13:18:58Z

Or I have to add "loggamma": "loggamma" without deleting the previous "loggamma": "lgamma", I tried this and this works perfectly fine.

This sounds ideal to me - this effectively does delete the previous entry.

abhaydhiman · 2020-08-09T13:23:03Z

Is it good to go now?

eric-wieser · 2020-08-09T13:28:18Z

No. You need to add "loggamma": "loggamma" to _known_functions_mpmath

abhaydhiman · 2020-08-09T13:33:04Z

oh! Sorry, I am on it.

eric-wieser · 2020-08-09T13:59:55Z

This fix now looks correct - can you add a test next to the other python code printer tests?

abhaydhiman · 2020-08-09T14:13:16Z

Testcase for this nsolve(im(loggamma(I*x)), x, 3.439*I) equation?

abhaydhiman · 2020-08-09T14:14:43Z

Also, do I have to add a test in test_pycode.py?

eric-wieser · 2020-08-09T14:17:55Z

Yes, please add a test to that file. Simply checking the result of something like pycode(loggamma(x)) should be enough, no need to complicate the test

abhaydhiman · 2020-08-09T14:20:08Z

Ok Thanks, can I make a separate function for that?

abhaydhiman · 2020-08-09T14:25:05Z

Or I have to add that in test_PythonCodePrinter

eric-wieser · 2020-08-09T14:27:51Z

test_MpmathPrinter looks like a good place to add it, since this is about mpmath.

eric-wieser

Release notes need updating to describe what was fixed (and not how), but the code is good.

abhaydhiman · 2020-08-09T15:22:54Z

But it got AssertionError. I don't know why that's happen

sympy/printing/tests/test_pycode.py

abhaydhiman · 2020-08-10T04:16:50Z

I have updated the release notes please review it. Thanks!!

eric-wieser · 2020-08-10T09:00:08Z

Still not quite right - will defer to @oscarbenjamin, since they reported the original bug.

oscarbenjamin · 2020-08-10T09:54:39Z

It looks like this fixes the original issue. What is not quite right?

Is it the release note?

Maybe it should say:

* printing
    * The mpmath code printer now correctly prints the loggamma function.
* utilities
    * Lambdifying an expression with loggamma using mpmath no longer raises NameError.

eric-wieser · 2020-08-10T09:57:01Z

Yeah, just the release note which decribed the diff not the result. That looks like a good note to me.

eric-wieser · 2020-08-10T09:57:25Z

Although I think it was ImportError

oscarbenjamin · 2020-08-10T10:26:32Z

Maybe there could also be a test for lambdify

abhaydhiman · 2020-08-10T11:00:13Z

So do I have to include a test for lambdify also?

abhaydhiman · 2020-08-11T05:12:04Z

@oscarbenjamin or @eric-wieser Do I have to add Test in test_lambdify.py file specifically in test_special_printers function if not please guide me where I have to add the Testcase for lambdify?

abhaydhiman · 2020-08-11T05:15:10Z

Is this lambdify(x, loggamma(x), 'mpmath')(5) test is valid?

oscarbenjamin · 2020-08-11T09:01:33Z

Yes, that seems good. You could test that it gives a couple of expected values for the loggamma function.

abhaydhiman · 2020-08-11T09:46:07Z

Yes, tried different values for x and it gives the expected output. So Is one test case is sufficient or I have to add more than one test for lambdify.

abhaydhiman · 2020-08-11T16:32:49Z

@oscarbenjamin I have added the test case please review it.

oscarbenjamin · 2020-08-11T16:38:04Z

Looks good

abhaydhiman · 2020-08-11T17:05:59Z

Thanks!

abhaydhiman · 2020-08-12T07:19:21Z

@oscarbenjamin any other change, you wanted in this PR.

Ayush-Malik

Looks good to me , @oscarbenjamin please review this PR

czgdp1807 · 2020-08-29T18:13:07Z

@oscarbenjamin Is this good to go?

oscarbenjamin · 2020-08-31T21:49:28Z

Looks good. Thanks!

eric-wieser reviewed Aug 7, 2020

View reviewed changes

sympy/utilities/tests/test_lambdify.py Outdated Show resolved Hide resolved

eric-wieser reviewed Aug 7, 2020

View reviewed changes

sympy/utilities/lambdify.py Outdated Show resolved Hide resolved

czgdp1807 added utilities utilities.lambdify labels Aug 8, 2020

eric-wieser reviewed Aug 9, 2020

View reviewed changes

sympy/printing/pycode.py Outdated Show resolved Hide resolved

abhaydhiman force-pushed the stage3 branch from 4b62881 to a574fd2 Compare August 9, 2020 13:22

abhaydhiman force-pushed the stage3 branch from a574fd2 to b470850 Compare August 9, 2020 13:37

Fixed issue number sympy#17411

b470850

Added testcase for loggamma

d466e06

eric-wieser approved these changes Aug 9, 2020

View reviewed changes

eric-wieser reviewed Aug 9, 2020

View reviewed changes

sympy/printing/tests/test_pycode.py Outdated Show resolved Hide resolved

Updated Testcase for loggamma

8052856

eric-wieser requested a review from oscarbenjamin August 10, 2020 08:59

Added a testcase for lamdify loggamma with mpmath

adf1b43

Ayush-Malik approved these changes Aug 18, 2020

View reviewed changes

oscarbenjamin merged commit f7c7217 into sympy:master Aug 31, 2020

oscargus mentioned this pull request Sep 5, 2021

Added loggamma to known_functions_mpmath #17425

Closed

lambdify loggamma now works for mpmath #19913

lambdify loggamma now works for mpmath #19913

Conversation

abhaydhiman commented Aug 7, 2020 • edited by eric-wieser Loading

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Aug 7, 2020 • edited Loading

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

abhaydhiman commented Aug 7, 2020

eric-wieser commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020

eric-wieser commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020

eric-wieser commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020 • edited Loading

eric-wieser commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020

eric-wieser commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020

eric-wieser commented Aug 9, 2020 • edited Loading

abhaydhiman commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020

eric-wieser commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020

abhaydhiman commented Aug 9, 2020

eric-wieser commented Aug 9, 2020

eric-wieser left a comment • edited Loading

Choose a reason for hiding this comment

abhaydhiman commented Aug 9, 2020

abhaydhiman commented Aug 10, 2020

eric-wieser commented Aug 10, 2020

oscarbenjamin commented Aug 10, 2020

eric-wieser commented Aug 10, 2020

eric-wieser commented Aug 10, 2020

oscarbenjamin commented Aug 10, 2020

abhaydhiman commented Aug 10, 2020

abhaydhiman commented Aug 11, 2020

abhaydhiman commented Aug 11, 2020

oscarbenjamin commented Aug 11, 2020

abhaydhiman commented Aug 11, 2020

abhaydhiman commented Aug 11, 2020

oscarbenjamin commented Aug 11, 2020

abhaydhiman commented Aug 11, 2020

abhaydhiman commented Aug 12, 2020

Ayush-Malik left a comment

Choose a reason for hiding this comment

czgdp1807 commented Aug 29, 2020

oscarbenjamin commented Aug 31, 2020

abhaydhiman commented Aug 7, 2020 •

edited by eric-wieser

Loading

sympy-bot commented Aug 7, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

abhaydhiman commented Aug 9, 2020 •

edited

Loading

eric-wieser commented Aug 9, 2020 •

edited

Loading

eric-wieser left a comment •

edited

Loading