Skip to content
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

Fix WorkSpace issues with expression optimization #535

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

jodavies
Copy link
Collaborator

@jodavies jodavies commented Jun 5, 2024

Here are two changes to make the optimizer easier to use.

The first is to dynamically allocate memory in Horner_tree, if the WorkSpace does not have enough space. There is no particular reason that the workspace should be used for the sort, here, other than that it exists, and might have capacity.

The second, is to re-use the same WorkPointer for every call of Generator when the optimized expression is placed into FORM's output. Generator modifies AT.WorkPointer internally, so each term which was added here was placed further and further into the buffer, unnecessarily.

In principle, if you arrive in generate_expression with a very-nearly full WorkSpace there could still be a crash. But this should be rare (?) .

These changes pass pySecDec's test suite.

@coveralls
Copy link

Coverage Status

coverage: 49.977% (-0.02%) from 49.999%
when pulling 603737d on jodavies:opt-dynamic-workspace
into 83e3d41 on vermaseren:master.

@jodavies jodavies modified the milestones: v5, v4.3.2 Nov 6, 2024
@jodavies jodavies requested a review from tueda November 11, 2024 14:10
@tueda
Copy link
Collaborator

tueda commented Nov 12, 2024

For the first commit, we can add a test, for example:

#:maxtermsize 200
#:workspace 50000
S x1,...,x19;
L F = (x1+...+x19)^4;
Format O1;
.sort
#optimize F
L G = `optimvalue_';
P G;
.end
assert succeeded?
assert result("G") =~ expr("389")

which fails for the master branch.

I am not 100% sure we can reuse the workspace in the second commit, but the absence of test failures suggests that it is working.

@tueda
Copy link
Collaborator

tueda commented Nov 13, 2024

The last commit (for check/fixes.frm) has a conflict but should be resolved easily.

@jodavies jodavies force-pushed the opt-dynamic-workspace branch from 11159ad to 950fad6 Compare November 13, 2024 16:20
There is no need to use the WorkSpace specifically for the sorting. If
it is too small, just allocate a new buffer and free it after.
Generator consumes WorkSpace, so here we run out very easily if we
don't reset the position that we use, each iteration.
@jodavies jodavies force-pushed the opt-dynamic-workspace branch from 950fad6 to b4925e2 Compare November 13, 2024 16:23
@coveralls
Copy link

Coverage Status

coverage: 50.199% (+0.09%) from 50.11%
when pulling b4925e2 on jodavies:opt-dynamic-workspace
into 3c5b5ce on vermaseren:master.

@jodavies jodavies merged commit bc0dae9 into vermaseren:master Nov 13, 2024
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants