LinearCombination: higher-dimension capable LLVM implementation#3533
Open
kmantel wants to merge 1 commit into
Open
LinearCombination: higher-dimension capable LLVM implementation#3533kmantel wants to merge 1 commit into
kmantel wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
This PR causes the following changes to the html docs (ubuntu-latest-3.11): See CI logs for the full diff. |
|
This PR causes the following changes to the html docs (ubuntu-latest-3.11): See CI logs for the full diff. |
jvesely
reviewed
May 10, 2026
| if len(param_type) == 1 and not isinstance(param_type.element, pnlvm.ir.ArrayType): | ||
| index = [ctx.int32_ty(0)] | ||
|
|
||
| if not isinstance(index, list): |
Collaborator
There was a problem hiding this comment.
It might be better to guarantee that index/indices is always a list.
Collaborator
Author
There was a problem hiding this comment.
I did that trying to deal with using lists as index in LinearCombination (ex L1568 exponent = self._gen_llvm_load_param(ctx, b, params, EXPONENTS, in_idx, 1.0) without changing other TransformFunctions or overriding _gen_llvm_load_param and couldn't think of something better. I'm fine changing it if it's better to do one of those or something else I haven't thought of.
|
|
||
| if isinstance(ptro.type.pointee, pnlvm.ir.ArrayType): | ||
| with pnlvm.helpers.array_ptr_loop(builder, ptro, f"combine_axis{len(indices)}") as (b, idx): | ||
| self._llvm_combine_body(b, ctx, vi, vo, val_f, pow_f, comb_op, params, [*indices, idx]) |
Collaborator
There was a problem hiding this comment.
Rather that recursing here, it might be clearer to use recursive_iterate_arrays over the output array.
Collaborator
Author
There was a problem hiding this comment.
Can this be used when exponents/weights are either scalar or matching the input shape? Not sure about it, but it seems that since recursive_iterate_arrays yields gep results, it would need to conditionally include or exclude those parameter vals in the recursive_iterate_arrays calls, and need to be repeated per combination.
Collaborator
Author
There was a problem hiding this comment.
@jvesely sorry, would you say this part is just unclear, or incorrect/costly/risky that it shouldn't wait to be reworked? I was a bit stumped on it considering my comment above. The overall support for >2d depends on these/similar changes
Collaborator
There was a problem hiding this comment.
Hi, sorry for the delay. I'll take another look at the updated changes today or tomorrow.
| # random 1/-1 | ||
| weights = 2 * (np.round(RANDh_V['weights'][variable.shape]) - .5) | ||
| if exponents == 'A': | ||
| exponents = RANDh_V['exponents'][variable.shape] |
Collaborator
There was a problem hiding this comment.
Why can't the above be passed as parametrized arguments?
Collaborator
Author
There was a problem hiding this comment.
They could be generated in the test too.
Otherwise, I think the issue is that they need to match [parts of] the variable shape, but AFAIK there isn't a way to couple parametrization vars (as in, only do (var1,exp1), (var2,exp2)... and not also (var1,exp2), (var2,exp1)), so I'd still need to pass some function or dict to get the correctly shape value out.
But yeah I can also pre-calculate these a bit better.
|
This PR causes the following changes to the html docs (ubuntu-latest-3.11): See CI logs for the full diff. |
kmantel
force-pushed
the
llvm-combine
branch
2 times, most recently
from
July 18, 2026 10:32
5148084 to
bd387a7
Compare
|
This PR causes the following changes to the html docs (ubuntu-latest-3.11): See CI logs for the full diff. |
|
This PR causes the following changes to the html docs (ubuntu-latest-3.11): See CI logs for the full diff. |
jvesely
reviewed
Jul 21, 2026
| self._gen_llvm_combine(builder, ctx=ctx, vi=arg_in, vo=arg_out, params=params) | ||
| return builder | ||
|
|
||
| def _gen_llvm_combine_body(self, builder, ctx, vi, vo, val_f, pow_f, comb_op, params, indices): |
Collaborator
There was a problem hiding this comment.
What is val_f ? if it's not a function, it probably shouldn't have the _f suffix.
Collaborator
Author
There was a problem hiding this comment.
Changed; it's not a function, I just misunderstood the naming convention I think
jvesely
reviewed
Jul 21, 2026
| tmp = (variable ** exponent) * weights | ||
| if operation == pnl.SUM: | ||
| expected = np.sum(tmp, axis=0) * scale + offset | ||
| if operation == pnl.PRODUCT: |
Collaborator
There was a problem hiding this comment.
might need to use elif, including
elif
assert False, "Unknown operation"
at the end.
Collaborator
Author
There was a problem hiding this comment.
Done here, though I copied this chunk from the other tests in test_combination.py and didn't touch those
jvesely
reviewed
Jul 21, 2026
| expected = np.prod(tmp, axis=0) * scale + offset | ||
|
|
||
| # wider tolerances needed for fp32 | ||
| np.testing.assert_allclose(res, expected, rtol=3e-5, atol=2e-7) |
Collaborator
There was a problem hiding this comment.
If the more lenient tolerance is only needed for fp32, please apply it only for fp32.
jvesely
reviewed
Jul 21, 2026
| # higher dimension arrays | ||
| test_varh1 = np.random.rand(1, 1, SIZE) | ||
| test_varh2 = np.random.rand(2, 3, SIZE, SIZE) | ||
| test_varh3 = np.random.rand(5, 4, SIZE, SIZE, SIZE) |
Collaborator
There was a problem hiding this comment.
Does using float32 dtype for the randomly generated values work to avoid having to relax tolerances when checking results?
Collaborator
Author
There was a problem hiding this comment.
Ah, yeah it does. I do have to do this for all of the random vars though, or some other variants of other tests here will go out of tolerance (at least test_reduce_function but I haven't run enough to see if it's always the same)
|
This PR causes the following changes to the html docs (ubuntu-latest-3.11): See CI logs for the full diff. |
supports parameter specifications as scalar, len-1 vector, or N-dim array matching shape of input (weights, exponents) or output (scale, offset) (len-M vector elementwise for (M, ...) shape arrays is intended to work, but does not work in non-compiled PNL)
|
This PR causes the following changes to the html docs (ubuntu-latest-3.11): See CI logs for the full diff. |
jvesely
reviewed
Jul 25, 2026
|
|
||
| assert isinstance(vi.type.pointee, pnlvm.ir.ArrayType) | ||
| with pnlvm.helpers.array_ptr_loop(builder, vi, "combine") as (b, idx): | ||
| in_idx = [idx, *indices] |
Collaborator
There was a problem hiding this comment.
how come "idx" is prepended to the list of indices? I'd expect the innermost loop to change the last index.
jvesely
reviewed
Jul 25, 2026
| if pytest.helpers.llvm_current_fp_precision() == 'fp32': | ||
| try: | ||
| res = res.astype(np.float32) | ||
| except AttributeError: |
Collaborator
There was a problem hiding this comment.
does np.random.default_rng().random(size=(*args), dype=np.float32) work without the extra scalar handling logic?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
supports parameter specifications as scalar, len-1 vector, or N-dim array matching shape of input (weights, exponents) or output (scale, offset)
(len-M vector elementwise for (M, ...) shape arrays is intended to work, but does not work in non-compiled PNL)