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

Add prolate spheroidal coordinates #220

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

adrn
Copy link

@adrn adrn commented Nov 13, 2024

I'm gearing up to add the Staeckel Fudge to Galax, so it would be useful to have a representation of prolate spheroidal coordinates. Transformations to this coordinate system need to specify a focal length parameter, Delta, so it's a bit different from other d3 representations. Thoughts on this approach?

@adrn
Copy link
Author

adrn commented Nov 13, 2024

Tests are failing for two reasons that might be related:

  • How can I tell coordinax that Delta is not a coordinate component and is instead a parameter that helps specify the coordinate system?
  • I can't get the self-transform to work (without explicitly specifying a value for Delta in represent_as) because something is inspecting the call signature and knows that that is a missing field in the dataclass.

One approach could be to allow a default Delta=None (because writing the coordinates themselves doesn't require a focal length, only transformations)?

@adrn adrn requested a review from nstarman November 13, 2024 16:40
@nstarman
Copy link
Contributor

nstarman commented Nov 13, 2024

How can I tell coordinax that Delta is not a coordinate component and is instead a parameter that helps specify the coordinate system?

That's a good question! We don't have a mechanism for that. This is complicated, essentially each value of $\Delta$ should result in a different representation. The non-viable but technically correct solution is to dynamically create a new class for each $Delta$. Not doing that, we could make $\Delta$ just be a property of the transformation, e.g. a mandatory kwarg to represent_as. However we still want to ensure math happens correctly, e.g. when 2 coords with different $\Delta$ are added it errors (or does some transformation then adds correctly). Therefore __add__ needs to know about $\Delta$, which can only happen if it's stored on the object. There are two options, both are a fair bit of work, and I'm not sure which is best.

  1. $\Delta$ is a field on the representation class. We would then need to change the fundamental assumption on representations, which is that all fields are coordinate axes. coordinax uses field_items() and related functions liberally to perform generalized operations on representations. We would need to define new functions like axis_field_items to replace the other functions everywhere (or leverage multiple dispatch as I discuss below).
  2. We define a new class (name TBD) RepDeltaWrapper that accepts the representation and the delta. The representation is still "pure" in that all fields are coordinates. RepDeltaWrapper would need to be made to work with the existing representation machinery. This might end up involving all the changes described for (1) + registering all the primitives.

So writing this out, (2) might be technically / mathematically better, but (1) seems best to do now.

Order of events: 😓

  1. Define an Axis field descriptor, like how I've done in astropy.cosmology.Parameter. This is used to annotate all the axis fields in coordinax (all of them currently). It should be parametric wrt the type it sets, so Axis[Distance] or Axis[Angle].
  2. Open a PR in dataclassish to define a filter flag type base class, like we did in unxt. The Flag ABC should error, the 'default' filter flag should be the same as if it weren't present. This is actually technically not needed because the multiple dispatch only cares about step 4. So maybe we just open an Issue on dataclassish for now.
  3. In coordinax define an AxesFilterFlag and register its behaviour for the field_items() etc functions. It can filter the dataclass' fields by whether they are annotated as Axis types in the class definition (they would still be Quantity on the instance).
    At this point all the existing representations should work completely unchanged since all their fields are axes.
  4. Define the prolate spheroidal coordinate classes, with Delta not as an axis. Register in all the behaviours for checking Delta when doing math operations, etc. Random extra thought: Delta should be a scalar only.

@nstarman
Copy link
Contributor

nstarman commented Nov 19, 2024

@adrn I started steps 1-3 in #225. The tests on using filtered axes is pretty minimal — just on FourVector — so there might be a couple functions I forgot to refactor. But this PR should now be able to define the Delta attribute!

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.

2 participants