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 AltAzCoord #35

Open
kiranshila opened this issue Jan 20, 2021 · 9 comments · May be fixed by #40
Open

Add AltAzCoord #35

kiranshila opened this issue Jan 20, 2021 · 9 comments · May be fixed by #40

Comments

@kiranshila
Copy link

To follow https://docs.astropy.org/en/stable/api/astropy.coordinates.AltAz.html

I'm currently tackling this, so PR inbound soon. I wanted to open an issue to start having discussions on this.

There are a ton of steps between local coordinates and the others. Astropy has only a few conversions fixed and uses a graph to find the shortest path between different coordinate systems. Should we do this as well? That would require a rewrite of the conversion methods. Additionally, as I'm getting into the weeds now reading how the conversions work, astropy uses calls to erfa for all the fast stuff. Is there a reason we shouldn't use the ERFA.jl wrapper? Or should we try a "pure julia" approach?

@kiranshila
Copy link
Author

For reference, for AltAz to Galactic, using the graph from https://docs.astropy.org/en/stable/coordinates/index.html#built-in-frame-classes,

we would have to implement AltAz -> CIRS -> ICRS -> FK5 -> Galactic

@mileslucas
Copy link
Member

The graph idea is really novel, I really like the idea of "looking up" the chain of functions that convert between coordinates. The implementation details are a bit more complicated than that, I assume.

Perhaps a good way to segment this work would be to initiate a PR for AltAz that enables conversion to/from ICRS and in a separate PR we can discuss the global conversion strategy in more detail. In particular, I'm also curious about the design decision for astropy to use a reference frame as a parameter for the coords rather than using unique coords classes. This kind of design decision is closely coupled with the conversion implementation.

@giordano
Copy link
Member

giordano commented Jan 21, 2021

Regarding the "why not using erfa", if we can do everything in Julia we can have advantages both in terms of speed and generality.

For the conversion graph, some eons ago, @helgee did Convertible.jl which I believe is somewhat related to this

@kiranshila
Copy link
Author

For the conversion graph, some eons ago, @helgee did Convertible.jl which I believe is somewhat related to this

This is a pretty cool package! It seems very similar, with much less complexity than what astropy is doing (it took me a few hours to work it all out).

With respects to the work breakdown, I agree. I can get a MWE of AltAz -> ICRS (I think we need CIRS for this, I can't find any examples of AltAz directly to ICRS).

Also, unlike astropy, I think we shouldn't allow construction of an AltAzCoord without an observation time and location. This never made sense to me, as its like underdefined from the perspective of a "Sky Coordinate"

@helgee
Copy link
Member

helgee commented Jan 21, 2021

Convertible.jl was quite the hack and I came up with a less dangerous solution in the meantime.

I have implemented a graph of time scales using ItemGraphs.jl for AstroTime.jl. Please have a look here.

Unfortunately, I will not be able to assist much in the coming weeks due to personal reasons.

@helgee
Copy link
Member

helgee commented Jan 21, 2021

Also have a look at the still unpublished AstroBase.jl. Some of the conversions have already been ported to Julia and you can lift them from there for the time being.

@kiranshila
Copy link
Author

Also have a look at the still unpublished AstroBase.jl. Some of the conversions have already been ported to Julia and you can lift them from there for the time being.

Oh interesting. You seem to have done a lot of the legwork there for the graph of frame conversion. Was the intention to use this library as the base package for Coords?

@giordano
Copy link
Member

@kiranshila would you be interested to work on this in the Google Summer of Code? I think you said you're a PhD student, if so you'd be eligible to participate

@kiranshila
Copy link
Author

@giordano That sounds interesting for sure! I'd have to clear things with an advisor if I start signing on for "formal deliverables", but I'll be working on this one way or another haha (because I desperately need it)

@kiranshila kiranshila linked a pull request Nov 7, 2021 that will close this issue
@giordano giordano linked a pull request Nov 7, 2021 that will close this issue
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 a pull request may close this issue.

4 participants