Added coefficient-origin support to exp_fields.cc

[The9Cat]

Describe your changes

This allows Gala to evaluate the EXP potential using the coordinate origin supplied by the coefficients

Details

The EXP ui assumes that the user wants field evaluations in the coordinate system of the expansion by default. For an absolute coordinate system with an arbitrary the expansion center, EXP needs to be told apply the translation. This is now done automatically for the getAccel() member which was added explicitly to support Gala so does not break EXP workflows and a new member, getFieldsOrigin() which is getFields() in the frame with the coefficient-origin translation.

Tests

Checked against Hayden Foote's MWE illustrating the issue

Note

This depends on the changes from EXP-code/EXP#219. When we are all in agreement, this PR will be merged into devel.

[lgarrison]

Thanks, Martin! I added a test for this and confirmed that it passes locally with your EXP change.

We'll also need to figure out what Gala branch to merge this into. Since it depends on EXP/devel, I think our idea from last year was that we would use Gala/devel. But that branch doesn't exist anymore; see #585.

So I think I would suggest that we either (1) bring back the gala/devel branch or (2) give up on the idea of running CI against a tagged EXP version and just always run CI against EXP/devel. Probably a question for @adrn.

[The9Cat]

Thanks.

Not sure about the best choice for branches. We could make an exception to the written EXP policy: main is stable and only gets bug fixes while devel may contains new features. Fixes to main are not supposed to make workflow-breaking changes to the API while devel is allowed to do that (although we'd prefer not to, of course). So I based this PR on devel since we added a new feature with a potential workflow-breaking change to any one who used getAccel() from pyEXP. Although one could argue that it fixes a bug and the API is backwards compatible even if the behavior is slightly different for getAccel().

Probably shouldn't over think this and do what makes the most sense to have EXP and Gala interoperate as intended.

Maybe @michael-petersen would like to comment on this?

[michael-petersen]

We've now set up EXP 7.10 to address this, which is currently EXP/devel.

I think it makes sense to point at the devel branch going forward for CI; it is meant to be stable but give us time to find bugs/issues. Is there any reason not to, @adrn, @lgarrison?

[The9Cat]

A follow up question/comment: it would be ideal to have either Gala or EXP (possibly both) to test compatibility as part of CI. Any thoughts on the best strategy for this?

I second Mike's though about Gala pointing at devel. For reference, the EXP-code policy is:

• main is the stable release. A user can rely on this to not change except for functional bug fixes.
• devel is the development branch. It is intended to contain working and tested new features as well as bug fixes.
• Features under development are in branches of devel; not required to be fully tested or working.

So pointing any Gala+EXP CI at devel would ensure that the code bases continue to work without surprise.

[lgarrison]

@adrn confirmed that the gala/devel deletion was accidental (probably automated by a PR closing), so I'll bring that back. But also, the EXP feature to support this PR has already landed in exp/main (EXP v7.10). So here's what I'll do:

• Update the EXP requirement to v7.10 in CI and the Gala docs
• Merge this branch into main
• Restore devel, branched off main
• Set up the weekly Gala tests to run on both main and devel, the latter against EXP/devel.

[lgarrison]

@adrn This should be pretty straightforward to review, the main thing to note is that the EXP version requirement is changing.

[adrn]

Right, thanks for the help on this @lgarrison - and thanks for the changes @The9Cat !