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

Extend wrapping of ERFA functions to AngleOps and SphericalCartesian #4571

Merged
merged 10 commits into from Mar 2, 2016

Conversation

Projects
None yet
8 participants
@jwoillez
Member

jwoillez commented Feb 5, 2016

One small step closer to making a full fledge ERFA available to python, somewhat related to #3123.

@jwoillez

This comment has been minimized.

Show comment
Hide comment
@jwoillez

jwoillez Feb 7, 2016

Member

I will continue adding tests to make coveralls happy. However, I have no idea where the sphinx error comes from. Could someone please help me and have a look? Thanks!

Member

jwoillez commented Feb 7, 2016

I will continue adding tests to make coveralls happy. However, I have no idea where the sphinx error comes from. Could someone please help me and have a look? Thanks!

@bsipocz

This comment has been minimized.

Show comment
Hide comment
@bsipocz

bsipocz Feb 7, 2016

Member

@jwoillez - It looks like that the numpy docs page was down, and the intersphinx reference file couldn't be downloaded, thus all of the cross references were raised as a warning. One should restart the build once numpy is back online.

(On a separate note: @astrofrog @eteq - do you think we should host a back-up intersphinx inventory in astropy-data? Sphinx supports to have multiple location listed in the mapping, and takes the first working one for each inventory).

Member

bsipocz commented Feb 7, 2016

@jwoillez - It looks like that the numpy docs page was down, and the intersphinx reference file couldn't be downloaded, thus all of the cross references were raised as a warning. One should restart the build once numpy is back online.

(On a separate note: @astrofrog @eteq - do you think we should host a back-up intersphinx inventory in astropy-data? Sphinx supports to have multiple location listed in the mapping, and takes the first working one for each inventory).

@jwoillez

This comment has been minimized.

Show comment
Hide comment
@jwoillez

jwoillez Feb 8, 2016

Member

@bsipocz - Thanks for the explanation! It look like things are working again.

All, this PR is ready for review. I hope you will spare me the -0.1% of coverage. ;)

Member

jwoillez commented Feb 8, 2016

@bsipocz - Thanks for the explanation! It look like things are working again.

All, this PR is ready for review. I hope you will spare me the -0.1% of coverage. ;)

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Feb 8, 2016

Member

@jwoillez - I am actually 👎 to this, at least in the astropy core itself. I could be missing something, but my read of AngleOps and SphericalCartesian is that it's basically a C-level library for angle and vector math. That's already in astropy as part of the coordinates package, but implemented in a more pythonic manner.

That said, it might make more sense for this to go into a dedicated pyerfa package (which could live in liberfa as a separate project, but could use the affiliated package template to minmize headaches ). Astropy would then just treat it as another extern.

Or am I misunderstanding the main purpose of these functions?

Member

eteq commented Feb 8, 2016

@jwoillez - I am actually 👎 to this, at least in the astropy core itself. I could be missing something, but my read of AngleOps and SphericalCartesian is that it's basically a C-level library for angle and vector math. That's already in astropy as part of the coordinates package, but implemented in a more pythonic manner.

That said, it might make more sense for this to go into a dedicated pyerfa package (which could live in liberfa as a separate project, but could use the affiliated package template to minmize headaches ). Astropy would then just treat it as another extern.

Or am I misunderstanding the main purpose of these functions?

@jwoillez

This comment has been minimized.

Show comment
Hide comment
@jwoillez

jwoillez Feb 10, 2016

Member

@eteq - I don't think I have the time ressources to go all the way to pyerfa, but this is a small step in that direction. Would looking at it form this angle be sufficient to accept this PR? We still have erfa in semi private astropy._erfa; there is therefore no intent to compete with what coordinates already does. This is just an increase of the exposure of erfa to python for those (like myself) who need direct access to erfa.

Member

jwoillez commented Feb 10, 2016

@eteq - I don't think I have the time ressources to go all the way to pyerfa, but this is a small step in that direction. Would looking at it form this angle be sufficient to accept this PR? We still have erfa in semi private astropy._erfa; there is therefore no intent to compete with what coordinates already does. This is just an increase of the exposure of erfa to python for those (like myself) who need direct access to erfa.

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Feb 11, 2016

Member

@jwoillez - I might see your point here, but I guess I don't understand in what circumstances this is useful on the python side. Isn't it all available already? Or are there some functions that have functionality missing from numpy/astropy/whatever is already available?

Member

eteq commented Feb 11, 2016

@jwoillez - I might see your point here, but I guess I don't understand in what circumstances this is useful on the python side. Isn't it all available already? Or are there some functions that have functionality missing from numpy/astropy/whatever is already available?

@mwcraig mwcraig added the coordinates label Feb 25, 2016

@hamogu

This comment has been minimized.

Show comment
Hide comment
@hamogu

hamogu Feb 28, 2016

Member

I have a feeling that @taldcroft would like this. It's probably a lot faster to get angular separations this way than through the coordinates package using compiled C code that's included in astropy already anyway.
See #3908

Member

hamogu commented Feb 28, 2016

I have a feeling that @taldcroft would like this. It's probably a lot faster to get angular separations this way than through the coordinates package using compiled C code that's included in astropy already anyway.
See #3908

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog Feb 28, 2016

Member

I would be interested to see some performance comparisons compared to the Python implementations we already use. For large arrays, there may be some nice performance improvements due to avoiding multiple copies of Numpy arrays. If this is faster, then I would be in favor of making use of this internally.

I agree we probably don't want to expose these though the public API, but if they make the code faster internally, why not?

Member

astrofrog commented Feb 28, 2016

I would be interested to see some performance comparisons compared to the Python implementations we already use. For large arrays, there may be some nice performance improvements due to avoiding multiple copies of Numpy arrays. If this is faster, then I would be in favor of making use of this internally.

I agree we probably don't want to expose these though the public API, but if they make the code faster internally, why not?

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Feb 28, 2016

Contributor

I think eventually one wants all erfa functions exposed to python in liberfa/erfa itself, maybe via a pyerfa subpackage or so, and then access it via something like astropy.extern.erfa. I can see using some of the vector matrix routines internally, although probably only in some performance-critical places, since there is considerable benefit to having the formalae explicitly written out.

Contributor

mhvk commented Feb 28, 2016

I think eventually one wants all erfa functions exposed to python in liberfa/erfa itself, maybe via a pyerfa subpackage or so, and then access it via something like astropy.extern.erfa. I can see using some of the vector matrix routines internally, although probably only in some performance-critical places, since there is considerable benefit to having the formalae explicitly written out.

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Mar 1, 2016

Member

@mhvk - I've come around to the idea of a liberfa/pyerfa or something like that, just as you say here. The downside is that it's yet more release management... But if it's tied to the liberfa release cycle it's not that much overhead.

I'm less sure we want to use this internally, though... I agree with @astrofrog that if there's a sizable performance increase it's worth it, but there is a real virtue in having the code be readable in python, which I think outweighs a modest performance increase.

Member

eteq commented Mar 1, 2016

@mhvk - I've come around to the idea of a liberfa/pyerfa or something like that, just as you say here. The downside is that it's yet more release management... But if it's tied to the liberfa release cycle it's not that much overhead.

I'm less sure we want to use this internally, though... I agree with @astrofrog that if there's a sizable performance increase it's worth it, but there is a real virtue in having the code be readable in python, which I think outweighs a modest performance increase.

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Mar 2, 2016

Contributor

OK, sounds like we all agree on the end goal. The more immediate question is what to do here; my feeling is to merge as is, since it brings liberfa closer and in the meantime doesn't really make the code base much larger anyway.

Contributor

mhvk commented Mar 2, 2016

OK, sounds like we all agree on the end goal. The more immediate question is what to do here; my feeling is to merge as is, since it brings liberfa closer and in the meantime doesn't really make the code base much larger anyway.

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Mar 2, 2016

Member

👍 from me for going forward with this PR, assuming it stays out of the public API.

I think a big role for a robust pyerfa wrapper is to provide meaningful function names. From that perspective I would agree that any ERFA functions that are visible currently using the completely cryptic original names should never be made public through astropy.

Member

taldcroft commented Mar 2, 2016

👍 from me for going forward with this PR, assuming it stays out of the public API.

I think a big role for a robust pyerfa wrapper is to provide meaningful function names. From that perspective I would agree that any ERFA functions that are visible currently using the completely cryptic original names should never be made public through astropy.

@mhvk mhvk added erfa and removed coordinates labels Mar 2, 2016

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

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Mar 2, 2016

Contributor

OK, merging! Thanks, @jwoillez!

Contributor

mhvk commented Mar 2, 2016

OK, merging! Thanks, @jwoillez!

mhvk added a commit that referenced this pull request Mar 2, 2016

Merge pull request #4571 from jwoillez/more_erfa
Extend wrapping of ERFA functions to AngleOps and SphericalCartesian

@mhvk mhvk merged commit 50bd068 into astropy:master Mar 2, 2016

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 76.619%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Mar 2, 2016

Member

Oops, shouldn't have merged that yet. As I said above I felt that this should not be merged until we know we're going to use it. Not an especially big deal at this point, though. I guess if we end up not using it we can take it out again.

Member

eteq commented Mar 2, 2016

Oops, shouldn't have merged that yet. As I said above I felt that this should not be merged until we know we're going to use it. Not an especially big deal at this point, though. I guess if we end up not using it we can take it out again.

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Mar 2, 2016

Member

In #4664 I created an issue to follow-up on this, so further discussion on actually using this can go there.

Member

eteq commented Mar 2, 2016

In #4664 I created an issue to follow-up on this, so further discussion on actually using this can go there.

@mhvk

This comment has been minimized.

Show comment
Hide comment
@mhvk

mhvk Mar 2, 2016

Contributor

@eteq - sorry for being too quick here. My sense was that the path we agreed on was that we do want a set of numpy-enabled python wrappers (pyerfa) eventually, which would wrap all routines, and that it was fine to merge that here in preparation for eventually moving over to liberfa. I don't know that there is any reason to revert: as is, I'd think we already cover quite a few astronomy routines which we end up not using.

Contributor

mhvk commented Mar 2, 2016

@eteq - sorry for being too quick here. My sense was that the path we agreed on was that we do want a set of numpy-enabled python wrappers (pyerfa) eventually, which would wrap all routines, and that it was fine to merge that here in preparation for eventually moving over to liberfa. I don't know that there is any reason to revert: as is, I'd think we already cover quite a few astronomy routines which we end up not using.

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Mar 2, 2016

Member

True enough - and I wasn't communicating too clearly what I was objecting to, so it's not a big deal. The primary downside is that it adds to the compilation time. But that might be a very small effect, I didn't look. Mainly this is my knee-jerk "don't add any more than necessary" attitude.

Member

eteq commented Mar 2, 2016

True enough - and I wasn't communicating too clearly what I was objecting to, so it's not a big deal. The primary downside is that it adds to the compilation time. But that might be a very small effect, I didn't look. Mainly this is my knee-jerk "don't add any more than necessary" attitude.

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog May 17, 2016

Member

@eteq - I'm marking this as 'no-changelog-needed' because it's private API, right?

Member

astrofrog commented May 17, 2016

@eteq - I'm marking this as 'no-changelog-needed' because it's private API, right?

@mhvk mhvk referenced this pull request May 25, 2016

Merged

fixed bugs in get_sun #4952

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