Repository navigation
Conversation
|
Travis is failing on 5/7 jobs in the build matrix, none of which seem to be related to the code in this PR. |
|
@andycasey it is likely to be a change in modeling which is currently undergoing restructuring. I'll have a look. |
|
I'm entirely in favor of adding this functionality, but I thought ERFA was supposed to include some of these conversions as low-level methods? I might be entirely wrong about that, but I wanted to raise the point in case someone knows of another implementation. pyslalib (https://github.com/scottransom/pyslalib) might also be useful for generating test cases. |
|
@andycasey it's the new jinja2 requirement from astropy. Will add to TRAVIS. |
|
@andycasey the structure of |
|
Ah: tests. Can you add some? |
There was a problem hiding this comment.
I would only allow an astropy.time.Time object. If float is really important, please use ducktyping to differentiate between time and float.
There was a problem hiding this comment.
I was following the convention set by the astropy.time and astropy.coordinates module where they would allow astropy objects or a float. I can ducktype for floatst if necessary, but checking against the Time baseclass would be sufficient to see if it's an astropy object or not
There was a problem hiding this comment.
For ducktyping I would use this piece of code (not doing type checks, such as type or isinstance:
if hasattr(dje, 'jd'):
jd = dje.jd
else:
jd = dje
|
@andycasey For now there are no things left to do from my side, right? Let me know if there's other questions. |
|
Yep, that looks fine; thanks for the feedback @wkerzendorf et al. I will get to this "soon". |
|
you mean "soon"(TM). 😉 . On a more serious note - there's no rush. |
|
This functionality looks useful. What's left to do? |
|
@rickyegeland see the inline comments |
|
Any chance this can updated, @andycasey ? Or plans to? This is something I think a number of people would be interested in and a great thing to have a version of even if it eventually gets replaced with something else. |
|
@crawfordsm - the machinery is now in place in |
|
@mhvk Oh good, I saw that was still on-going but had missed that was merged. My guess is with the coordinates is probably fine, however the need for the convenience function is critical. |
|
Following the additions @mhvk has made to astropy 1.3, the convenience function is very simple. This gist should cover it. Although you might want to change the function definition, for example to allow the user to set the ephemeris used to calculate the velocities, a la In terms of where we put this, just for consistency maybe it ought to be a method of the |
|
Okay, perhaps we should move the discussion of where it should go back to astropy/astropy#3544, where it might be more useful to have it then here (Also yes truly awesome work by @mhvk and others to see this finally get implemented and how relatively few lines of code it takes). I'll leave it up to @andycasey if he would like to see this still merged or closed. I think there is some value in having this in terms of another implementation at the very least for testing/comparison purposes (which will need to be extensive for this functionality) even if it gets replaced quickly by other implementations. |
|
@andycasey the |
|
@migueldvb yes, that's right. Now that there is a better, Correct(tm) way to do barycentric/similar correction in astropy, then I think this should be closed. Part of the motivation for opening this was because I knew this would be something that people would immediately want when switching to astropy, and didn't already exist. At the time there was talk that this could be incorporated within the astropy ecosystem in a better way, but there wasn't a clear path forward. Hopefully this code was useful to some in the meantime. I'd am going to close this, and recommend that a new PR is opened that expands on @StuartLittlefair's gist. Re-open if there is disagreement! Thanks all |
Listen and add new components to layout dropdown.
9 participants
Hola,
This pull request adds a utility function that will calculate heliocentric velocity corrections for astronomical sources. The
helcorrandbaryvelfunctions have been updated from the astrolibpy package which was written by Sergey Koposov (@segasai). Those astrolibpy functions were originally Fortran functions from the triassic period before they were updated to modern IDL. Inhelcorr.pyI have credited authors who have contributed to these functions over the years, and @segasai has granted permission for these updated functions to be included in AstroPy.If people have strong opinions about the input to the
helcorrfunction then I'd like to hear them. At the moment it takes observatory longitude, latitude, altitude, source RA, source Dec and MJD, but in reality these could be replaced with just aastropy.coordinates.SkyCoordand aastropy.time.Timeobject with a specifiedlocation. What do people think?We also need to drum up some test cases for this code. Perhaps someone would be kind enough to generate a few test cases using IRAF?