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
[WIP] Fix digamma integral #26563
base: master
Are you sure you want to change the base?
[WIP] Fix digamma integral #26563
Conversation
Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes. ❌ There was an issue with the release notes. Please do not close this pull request; instead edit the description after reading the guide on how to write release notes.
Click here to see the pull request description that was parsed.
|
def _eval_is_finite(self): | ||
z, s, a = self.args | ||
if z == 0 or a == 0: | ||
return z.is_zero and a.is_zero and s.is_nonpositive |
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.
What should happen here if any of the is_
checks gives None
?
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.
Hi @oscarbenjamin , thanks for taking a look and the information about the booleans.
I thought python and was compatible with the fuzzy logic but it is indeed not. I will use the fuzzy_and function, this should handle correctly the None cases.
Also, I am not sure what is the convention when the function evaluates to NaN, should the is_finite be None?
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.
what is the convention when the function evaluates to NaN, should the is_finite be None?
Probably. What case evaluates to nan that you are thinking of?
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.
It is because a lot of situations where the function is infinite is because one has s.is_nonpositive
gives False when s is not real. I have added a check for s to be real, when s is complex the expression is undefined so now the function should return None.
Fixed some edge cases. Check that s is real for 0**s. Return None when the arguments are infinite.
exp_expr = self.expand(func=True) | ||
if exp_expr != self: | ||
return exp_expr.is_finite |
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.
What is expand(func=True)
being used for here?
We should avoid creating new expressions during an assumptions query. Just creating the new expression runs the risk of triggering an identical assumptions query which leads to infinite recursion.
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.
Ok, I did not think about that. My idea was that, since LerchPhi includes a lot of common functions as particular values (zeta function, polylog, Dirichlet eta, ...), I can try to write the function in terms of these other functions and then let them determine whether the function is finite or not. Is there a safe way to do it without the risks you are mentioning?
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.
There is not really a safe way to do it. We just have to accept that the core assumptions cannot resolve all queries:
https://docs.sympy.org/latest/guides/assumptions.html
if a.is_zero is False: | ||
return True | ||
if s.is_real: | ||
return s.is_nonpositive |
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.
I'm not sure if this works correctly when a.is_zero is None.
References to other Issues or PRs
Fixes #26523
Brief description of what is fixed or changed
Added regression tests
Other comments
Release Notes