feat: add separateAllVarsWithDims option to spec file to separate all vars with a dimension on the LHS

[ToddFincannonEI]

Fixes #601

This PR proposes an alternative to the specialSeparationDims syntax to avoid having to maintain the list in cases where all variables containing a given subscript should be separated.

The spec file can now contain a property named separateAllVarsWithDims with an array value. Each entry is either a dimension name string in canonical format, or an array of dimension names.

If a variable LHS matches all dimension names in one of the separateAllVarsWithDims entries, then it is separated on those dimensions. If a variable is specifically called out in specialSeparationDims, then that takes precedence, and separateAllVarsWithDims is not checked. It does not make sense for a variable separation to be listed in both formats.

I added a test model called separateall.mdl that has two apply-to-all arrays named a and b.

DimA: A1, A2 ~~|
DimB: B1, B2 ~~|
a[DimA] = c ~~|
b[DimB] = c ~~|
c = 42 ~~|

Without a spec file, the following code and variable listing is generated.

// a[DimA] = c
for (size_t i = 0; i < 2; i++) {
_a[i] = _c;
}
// b[DimB] = c
for (size_t i = 0; i < 2; i++) {
_b[i] = _c;

a[DimA]: aux
= c
refId(_a)
families(_dima)
subscripts(_dima)
hasInitValue(false)
refs(_c)

b[DimB]: aux
= c
refId(_b)
families(_dimb)
subscripts(_dimb)
hasInitValue(false)
refs(_c)

I made a spec file that tests both separation methods:

"specialSeparationDims": { "_a": "_dima" },
"separateAllVarsWithDims": ["_dimb"]

The variables are separated in both cases:

// a[DimA] = c
_a[0] = _c;
// a[DimA] = c
_a[1] = _c;
// b[DimB] = c
_b[0] = _c;
// b[DimB] = c
_b[1] = _c;

a[DimA]: aux (non-apply-to-all)
= c
refId(_a[_a1])
families(_dima)
subscripts(_a1)
separationDims(_dima)
hasInitValue(false)
refs(_c)

a[DimA]: aux (non-apply-to-all)
= c
refId(_a[_a2])
families(_dima)
subscripts(_a2)
separationDims(_dima)
hasInitValue(false)
refs(_c)

b[DimB]: aux (non-apply-to-all)
= c
refId(_b[_b1])
families(_dimb)
subscripts(_b1)
separationDims(_dimb)
hasInitValue(false)
refs(_c)

b[DimB]: aux (non-apply-to-all)
= c
refId(_b[_b2])
families(_dimb)
subscripts(_b2)
separationDims(_dimb)
hasInitValue(false)
refs(_c)

Because generation and execution do not fail in this test model, I added a check script that generates the model listing and confirms that both variables are separated. This is the only way to test the PR, since we don't have a failing test otherwise.

I tested this change on an EPS 4 model and confirmed that the new spec file format results in the same C code generation as the previous spec file that listed each variable separately.

[chrispcampbell]

@ToddFincannonEI: I pushed some additional tests and comment changes, and added some inline comments here to call some things to your attention. Given that this is kind of a hidden feature for now and probably only used by EPS, it's probably good enough but will let you decide.

The main thing I want to hear back from you is about the case of variables with more than one dimension; the code seems to be set up to accept that case but there wasn't a test for this other than the small one I added so I want to make sure the way I tested it is how you expected it to work, or if there are other cases we should add. I'll wait for your feedback before I mark it approved.

[chrispcampbell]

We had an existing unit test (or something closer to a unit test) for specialSeparationDims and I think we should do the same for this new separateAllVarsWithDims so I added them. I always prefer to have vitest tests defined for things as the iteration cycle is much tighter than having to rely on sample model tests.

[ToddFincannonEI]

These are better tests than the model test I added. I should start doing it that way. Shall we remove the model test, since the unit test covers the same thing?

The tests work the way I expected. I am thinking of separateAllVarsWithDims as a kind of shorthand for specialSeparationDims. They both have the effect of setting separationDims for the subscriptPositionsToExpand call, so the dim list has the same semantics.

[chrispcampbell]

Shall we remove the model test, since the unit test covers the same thing?

No, I think there's value in having both kinds of tests. The model test helps verify things end-to-end (like, that the spec file is passed through the chain as expected).

[chrispcampbell]

I changed the type to string[][] to make it slightly more obvious that how this works in the case of providing more than one dimension.

[chrispcampbell]

I think it would be better to avoid having the implicit "up convert string to string[]" code so that you always have to provide a string[][] in the spec file, but I didn't make any changes to the code. I guess I'm fine leaving it as is given that this is basically a hidden feature.

[ToddFincannonEI]

I was following the implementation we had for specialSeparationDims. I agree it would be safer to force the type, but in practice, it's nice to have the single string option.

[chrispcampbell]

I added a TODO here because it's possible that you could end up with generated vars that are not separated as expected. In practice I don't think this is likely to happen given that defineVariables is typically used in cases where the variables are already non-apply-to-all, but I could be wrong about that. Anyway, I think it's fine to not act on this now but wanted to flag it in case you think we should fix it now (it wouldn't be too hard to pass the spec to this function).

[chrispcampbell]

Based on the discussion in the comments, I think this is fine to merge in its current state. As noted, I think there's value in having both kinds of tests, and no urgent changes needed in the code, so I will merge this now and publish a release.