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

Determine if use of ERFA geometry functions in coordinates is a good idea #4664

Closed
eteq opened this Issue Mar 2, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@eteq
Member

eteq commented Mar 2, 2016

As discussed in #4571, it might make sense to use some of the angle and geometric operations that are built into ERFA to speed up some of the coordinate operations. However, there's currently a question whether there's a significant performance improvement. Opinions varied somewhat in #4571, but at least my opinion was that we should only do this if there's a significant improvement, and otherwise stick with the pure-python/numpy machinery that currently exists. If we do end up not using any of it, we may want to revert #4571 prior to release (although it should definitely go into a hypothetical future erfa/pyerfa external library).

cc @mhvk @taldcroft @jwoillez

@eteq eteq added the coordinates label Mar 2, 2016

@eteq eteq added this to the v1.2.0 milestone Mar 2, 2016

@eteq eteq changed the title from Determine if use of ERFA geometry functions is a good idea to Determine if use of ERFA geometry functions in coordinates is a good idea Mar 2, 2016

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Mar 2, 2016

Member

On the philosophical side, I don't entirely agree that we should always prefer a pure-Python "readable" implementation over a black-box library, IF that library is already available and assumed to be well-trusted and validated. In other words I don't see a really good reason to re-invent any SOFA wheels which provide exactly the functionality we need purely for the sake of having a readable implementation. Astropy is first and foremost a production library, and as it matures I think we need to take core performance as a higher priority.

Now of course much of what astropy does is provide a far richer API for complex behaviors, but for low-level grunt work I would argue that ERFA should do the heavy lifting. That's why people make standard libraries, so code can be re-used. However, having said this, I don't know what practical impact the new functions have on existing or new code in coordinates. (I just haven't really looked in detail).

Member

taldcroft commented Mar 2, 2016

On the philosophical side, I don't entirely agree that we should always prefer a pure-Python "readable" implementation over a black-box library, IF that library is already available and assumed to be well-trusted and validated. In other words I don't see a really good reason to re-invent any SOFA wheels which provide exactly the functionality we need purely for the sake of having a readable implementation. Astropy is first and foremost a production library, and as it matures I think we need to take core performance as a higher priority.

Now of course much of what astropy does is provide a far richer API for complex behaviors, but for low-level grunt work I would argue that ERFA should do the heavy lifting. That's why people make standard libraries, so code can be re-used. However, having said this, I don't know what practical impact the new functions have on existing or new code in coordinates. (I just haven't really looked in detail).

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Mar 2, 2016

Contributor

The other question is whether the python wrappers should end up just in astropy, or rather be exposed in liberfa via a "pyerfa" package or so. I'd like the latter, and commented so in #3123 as a reminder of that option.

Contributor

mhvk commented Mar 2, 2016

The other question is whether the python wrappers should end up just in astropy, or rather be exposed in liberfa via a "pyerfa" package or so. I'd like the latter, and commented so in #3123 as a reminder of that option.

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Mar 3, 2016

Member

The other question is whether the python wrappers should end up just in astropy, or rather be exposed in liberfa via a "pyerfa" package or so.

While in the past I have said I don't want to do that, I've come around to @mhvk side here - a pyerfa or similar as a separate package makes sense, and we can easily just put it in astropy/extern with little-to-no change in how astropy uses it.

Member

eteq commented Mar 3, 2016

The other question is whether the python wrappers should end up just in astropy, or rather be exposed in liberfa via a "pyerfa" package or so.

While in the past I have said I don't want to do that, I've come around to @mhvk side here - a pyerfa or similar as a separate package makes sense, and we can easily just put it in astropy/extern with little-to-no change in how astropy uses it.

@eteq eteq modified the milestones: v1.3.0, v1.2.0 Jun 12, 2016

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Nov 29, 2016

Contributor

It seems clear this isn't going to happen for 1.3, so I'm removing the milestone. (There isn't a 1.4 yet, but I think in any case issues that do not require action should not have a milestone associated with it.)

Contributor

mhvk commented Nov 29, 2016

It seems clear this isn't going to happen for 1.3, so I'm removing the milestone. (There isn't a 1.4 yet, but I think in any case issues that do not require action should not have a milestone associated with it.)

@mhvk mhvk removed this from the v1.3.0 milestone Nov 29, 2016

@mhvk mhvk added this to the Future milestone Nov 29, 2016

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Jul 25, 2018

Contributor

#7639 is using erfa for speed-up.

Contributor

mhvk commented Jul 25, 2018

#7639 is using erfa for speed-up.

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Aug 4, 2018

Contributor

Closing, since erfa geometric functions are now used, so we need to include them (#7639)

Contributor

mhvk commented Aug 4, 2018

Closing, since erfa geometric functions are now used, so we need to include them (#7639)

@mhvk mhvk closed this Aug 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment