-
Notifications
You must be signed in to change notification settings - Fork 154
fix: function and arg size book-keeping for TestSub recursion - #874
fix: function and arg size book-keeping for TestSub recursion #874jodavies wants to merge 2 commits into
Conversation
jodavies
commented
Jul 13, 2026
Tentatively, this is also a 1% performance improvement in forcer, forcer-exp and maybe hyperform.
coverage: 65.044%. first build — jodavies:issue-741-gpt into form-dev:master
I put back subsubveto, since @tueda is not sure about it. I think it would help if we could find a FORM script which would hit the following code, in the case that AN.subsubveto == 1:
Lines 741 to 748 in b143fc2
While this fixes many examples, I think it the code is still not correct, but for slightly independent reasons. @cbmarini has been also looking at an issue with this same code:
CFunction rat;
Symbol ep;
Local F = rat(70944*ep^5,1);
Print "%r";
Multiply replace_(ep,100000);
Print "%r";
Print;
.end
Here the issue is the in-place call of Normalize here
Line 1660 in b143fc2
In this case, Normalize makes the term larger after raising 100000 to the fifth power: thus Normalize blindly over-writes the trailing term information.
We need to either, copy the term to the workspace and normalize there, and then copy back taking care of the size changes, or not Normalize at all at let the later Normalize during sorting deal with the terms. Then one has to instead make sure that terms which go to 0 are still handled correctly in the Nest stack book-keeping (which was why the Normalize was introduced in the first place).
jodavies
commented
Jul 16, 2026
The second commit resolves the problem by not calling Normalize at all, during the recursion, but relying on the Normalization just before the following sort. The Nest stack is updated entirely by the recursive calls. I am not sure if it is guaranteed that TestSub never increases the size of the term, this does not happen in the test suite at least. The commit adds a Terminate in this case.
As far as I can tell, the code has now come "full circle" back to 2003, except that "subsubveto" is still there?
Any thoughts on this?
fcfd56b to
9b63dd6
Compare
0206c08 to
f501713
Compare
jodavies
commented
Aug 22, 2026
This is not yet done: the Issue741_2 test but with a single replace_, as in
Multiply replace_(x,0
,y,12345
,k1,4321*k2
,p1,p2
,p3,k1-k2
,z,1000000);
Hits the new TestSub: term size increased during recursion. termination. (And of course, it also crashes without the commits of this PR).
f27edcc to
3b806e1
Compare
When TestSub is called recursively, correctly update the nested sizes by relying on the recursion to fix the sizes at each level. Remove the in-place call to Normalize: this is dangerous, since it may increase the size of the sub-term, which destroys trailing term data. Normalize is called once, in separate memory, when the terms are stored for sorting. This fix was prepared with the help of an LLM report: https://gist.github.com/tueda/9d71a87e04dcc1f51fbbb432ae1af903 Its findings are in line with previous debugging efforts by me, but I did not figure out a proper fix at the time. In addition: - add a check that we do not exceed the FunctionLevels setup parameter, and give an error message for the user if so. - check RecFlag has the same value on entry and when returning from TestSub
This commit fixes cases where terms change size deeper in the nested structure, and following the calls to Normalize and Sort, the nesting stack is already updated. We must not update it a second time, in the recursion. Includes the test case which requires this fix. This fix also follows from an LLM report: https://gist.github.com/jodavies/696dc33b34819ce58c29f3356d2d835e
3b806e1 to
b7b711d
Compare
jodavies
commented
Sep 3, 2026
Unless anyone can think of more tests which should be added, we can merge this one now. This part of the code is much better tested than before, and it fixes many cases which previously gave bad results or crashed.
I don't measure any performance regression.
When TestSub is called recursively, correctly update the nested sizes. Rely on the recursion to fix the sizes at each level, except when an argument term disappears completely; then we fix up the whole nesting stack and continue processing further argument terms.
This fix was prepared with the help of an LLM report: https://gist.github.com/tueda/9d71a87e04dcc1f51fbbb432ae1af903
Its findings are in line with previous debugging efforts by me, but I did not figure out a proper fix at the time.
In addition:
This fixes #367 and #741