-
-
Notifications
You must be signed in to change notification settings - Fork 62
respect $IterationLimit more places #1477
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
3be2a1b to
6aaacb0
Compare
mathics/builtin/numbers/calculus.py
Outdated
| # Also, we can get either a MaxIterations failure, *or* a $IterationLimit error. | ||
| # | ||
| # For a not so well behaving function, the result can be less accurate: | ||
| # >> FindMaximum[-Exp[-1/x^2]+1., {x,1.2}, MaxIterations->10] |
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.
In[4]:= FindMaximum[-Exp[-1/x^2]+1., {x,1.2}, MaxIterations->2]
FindMaximum::cvmit:
Failed to converge to the requested accuracy or precision within 2
iterations.
Out[4]= {1., {x -> 0.168461}}
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. For FindMaximum, at iterations 2 it fails. At 10 though it doesn't which is what the test was.
Also, when it fails, it gives a value, and Mathics3 does not.
|
@mmatera: I am seeing here that we get a lot of Iteration limit occurrences in FindRoot, and one in combinatorica testing. I haven't had time to track these down. But there is a bit here that should be done independent of improving $InterationLimit. As too often is the case, in looking at failures, there is a bit of other stuff that should be improved. In particular, I think we should set the IterationLimit to 4096 as it is in WMA. And then to deal with the slowness of tests that were we to hit 4096, we set that in a Module with a local setting. So at some point I'll split this PR in two. |
However also go over adjust tests so that they don't slow down.
Co-authored-by: Juan Mauricio Matera <matera@fisica.unlp.edu.ar>
5fab132 to
5fc89db
Compare
|
|
||
| # iteration is used to keep track of evaluation chains and is | ||
| # compared against $IterationLimit. | ||
| self.iteration_count = 0 |
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.
In MathicsSession, the same evaluation object is used in all the evaluations of the session. At which point self.iteration_count is reset to 0?
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 - thanks. So we should reset this on each test.
We've added a number of other fields too, so those should probably be reset as well.
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.
In the old Mathics, one Evaluation object was created each time a line in the CLI or a cell in the web interface was executed. At some point, we change this behavior because most of the time, the Evaluation object does not change between evaluations. Now, since the evaluation always starts with a call to evaluation in the MathicsSession object, it is sufficient to reset the value there.
With this, it seems all the tests pass now
|
@mmatera if this is okay to go as one PR, great. If not, I'll split it in two tomorrow along the lines mentioned before. |
I think this is OK for now. Then, I can try to fix the output of |
|
LGTM. @rocky, merge this when you feel it is ready. |
Fixes #1476
There are probably more places where we need to do something similar, but this is a start.