-
-
Notifications
You must be signed in to change notification settings - Fork 61
First cut at Hypergeometric2F1 and some other Hypergeometric function revisions #1431
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
Conversation
d686da3 to
c46f005
Compare
Also some results were corrected by making more use of SymPy's hyperexpand function.
4e51c61 to
0c9c25e
Compare
|
I'll take a look soon. |
f136680 to
a2f2cba
Compare
a2f2cba to
1c84228
Compare
List of issuesThis comment is just a log of potential issues found during review and is being worked on. The final comments will be added to the code or in a separate comment later.
|
I wonder if it is not wrong but just different from WMA. There could be several valid solutions.
I tracked down what's going on in the code, and these results are coming from SymPy's For So, one possibility is to add the same thing for Hypergeometric1F1 with a suitable comment explaining why this is done rather than take SymPy's answer. I am assuming there's no way to tell SymPy to return 1.
Here, the difference has to do with whether the result is an Integer or a Machine number. This, I think, is easily fixed. |
based on Aravindh's review
In commit 101ee0a I've addressed these specific problems and a couple more. However, I can see that getting everything the same as WMA, including its particular level of error handling and how it treats lists in So we might have to note down problems, but defer handling these for later. Please use your judgment and experience. If something is important and wrong, let me know. If it is more of a quirky kind of behavior that feels more like this is just the way WMA happens to work, note it down, but we'll fix some other time. (For example:
|
Thank you and I fully agree. I will take a look with this advice in mind. |
|
Very sorry, the latest commit is due messing around with the |
No problem. Would much rather have this than have apathy :-) |
| evaluation.message( | ||
| "HypergeometricPQF", | ||
| "hdiv", | ||
| Expression(Symbol("Hypergeometric"), a, b, z), |
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 will be revising this to use Pattern[] in the signature docstring as described in Mathics3/Mathics3-development-guide#44 to reduce the clumsiness of having to recreate an expression that already exists.
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 will be revising this to use
Pattern[]in the signature docstring as described in Mathics3/mathics-development-guide#44 to reduce the clumsiness of having to recreate an expression that already exists.
@aravindh-krishnamoorthy #1440 is how to approach this by introspecting Python's call stack to pick out the Expression that must already exist. More specifically, line 209 would become:
from mathics.eval.stackframe import get_eval_Expression
...
evaluation.message("HypergeometricPFQ", "hdiv", get_eval_Expression())
But let's not hold up this PR due to the discussion of that PR and whether we might change the docstring to "Pattern[Expression, ... " as has also been proposed.
I will revise that separately based on the results of the discussion.
New options booleans ShowRewrites and ShowEvaluation can be added to allow filtering TraceEvalation output to filter other either rewrites rules or evaluation expressions. Presumably you don't want to filter both.
Introduce `get_eval_Expression()` `get_eval_Expression()` can be used in Mathics3 Builtin evaluation methods to find the Expression that caused the evaluation to get triggered. mathics.file_io.files module converted to use this function to test worthiness.
Also some results were corrected by making more use of SymPy's hyperexpand function.
based on Aravindh's review
…ore into add-Hypergeometric2F1
|
@aravindh-krishnamoorthy, this has been pending for a while, and I'm afraid the longer it remains pending, the harder it will be to merge. If there are concerns or comments, let me know. |
|
I'll take a closer look tomorrow and let you know if any issues I find. |
In addition to adding Hypergeometric2F1, we start moving code to
mathics.eval.specialfns.hypergeomFixes #1187
Most of the eval functions have been moved out of
mathics.builtin.specialfns.hypergeom. And in the process, we expand hyper results to match what WMA does more closely. Some prior erroneous evaluation was therefore corrected.However, this PR is already large, so Hypergeometric2U will be handled separately.
Moving eval code for
MeijerG, was attempted, but that causes a weird interaction withtest/doc/doc_common.py, which somehow makes it think that hypergeom is included more than once.