-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Time scalar & multi-dimensional, using new ERFA #3138
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
Conversation
|
p.s. I'm not quite sure what the automatic failure is about. Will investigate later, but for now maybe it is just as well that travis doesn't burn cycles on this -- mostly posting this for a first look to see whether there is agreement on the direction. |
13499bb to
d3dbcc8
Compare
|
@jwoillez - to get |
|
@taldcroft - I now have all tests passing with a much simplified way of handling scalars (since One particular change that I made is to treat input of |
astropy/erfa/erfa.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the driver for making the STATUS_CODES values unicode here and elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@mhvk - this looks awesome, thanks! So the main thing is to add tests for multi-dim as you said. I think there also need to be a bit more depth in testing the scalar case, especially for |
ed34808 to
f7c59ee
Compare
|
Yes, I'll try to add some more scalar tests as well; for these, |
36dd832 to
870c238
Compare
|
OK, the multidimensional |
b287725 to
afc1af8
Compare
|
OK, after rebasing with #3166 in, this no longer depends on how |
e55871d to
c8eaddb
Compare
|
Finished code review and it looks great (once Travis passes). Just need to add some mention of the multi-dim capability and broadcasting of inputs in the docs. The "Scalar or Array" section would be a good place. And of course CHANGES.rst. |
|
Thanks! I added documentation and hopefully resolved the numpy <= 1.8 bug (the earlier solution of which disappeared with my rebase...). I realised we could take this further and define |
|
@taldcroft - do you want to have a quick look at the last commit with the documentation? I think this is ready to go in. |
|
@taldcroft - just in case: don't merge this yet -- waiting for #3173 to go in first... |
7fab31a to
59183e9
Compare
|
@taldcroft - ok, now rebased again, and it should really be all OK. Maybe just have a quick look over the documentation. |
|
Docs look good, but it seems you need to rebase again. |
CHANGES.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you are rebasing, I would suggest .. handle arbitrary array dimensions .. just to be a little more clear.
Ensure datetime.timedelta only is multiplied with integers (python2 cannot handle float)
59183e9 to
e915e3d
Compare
|
👍 feel free to merge! |
Time scalar & multi-dimensional, using new ERFA
|
Assuming this is a new feature since Astropy 0.4; i.e. new functionality that did not previously exist, this should be labeled Affects-release, not Affects-dev. Affects-dev means "this pertains to an issue that will only ever be seen by people developing Astropy, not users of released versions". |
|
OK, here I was thinking |
|
In a way it does--it provides an enhancement over existing releases. |
|
These categorizations just help me figure out what needs to go where, and also helps me a lot when preparing a release to make sure everything's in its right place. |
With the new erfa routines, scalars are OK, and hence much of the code required in
Timeto keep track of scalar values could be removed. At the same time, arbitrary dimensions become possible:This is work in progress mostly since tests of the multidimensional behaviour are still missing (though it passes all normal tests!). @taldcroft - please have a look!
p.s. This relies on #3135; check the second commit to see just the changes in
Time. EDIT -- this now stands on its own, as the required changes to ERFA were part of #3141