Skip to content

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Nov 24, 2014

With the new erfa routines, scalars are OK, and hence much of the code required in Time to keep track of scalar values could be removed. At the same time, arbitrary dimensions become possible:

In [1]: from astropy.time import Time; import numpy as np, astropy.units as u

In [2]: t = Time(50000.0, format='mjd')

In [3]: t2 = t + np.arange(6.).reshape(3,2) * u.day

In [4]: t2
Out[4]: 
<Time object: scale='utc' format='mjd' value=[[ 50000.  50001.]
 [ 50002.  50003.]
 [ 50004.  50005.]]>

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

@mhvk
Copy link
Contributor Author

mhvk commented Nov 24, 2014

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.

@mhvk mhvk force-pushed the time-scalar-done-easy branch 2 times, most recently from 13499bb to d3dbcc8 Compare November 29, 2014 17:40
@mhvk
Copy link
Contributor Author

mhvk commented Nov 29, 2014

@jwoillez - to get Time to work properly with the new erfa, including for scalars, I had to make a small change to how scalars are handled for numpy <1.8 (mhvk@ed34808). I use reshape instead of getting an item, which ensures that the output remains an ndarray (as happens for numpy >1.8). Could you have a look? If you agree this is sensible, I can add some erfa test cases here (or split out into a new PR).

@mhvk
Copy link
Contributor Author

mhvk commented Nov 29, 2014

@taldcroft - I now have all tests passing with a much simplified way of handling scalars (since erfa can now handle them), and this also allows arbitrary dimensions in Time. I will write some test cases for the latter, but in the meantime, it would be useful if you could have a look.

One particular change that I made is to treat input of Time objects just like any other, letting it become an array of with object dtype.

@mhvk
Copy link
Contributor Author

mhvk commented Nov 29, 2014

@eteq - just to be clear, while my change to erfa.py.templ is small, the propagation to erfa.py means there are many changes there. So if I understand correctly, from a don't-make-git-history-overweight perspective, it might be advantageous to wait for #3159 before merging this?

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I just ran python cython_generator.py -- I now tried python3 cython_generator.py and those u disappear. @jwoillez, @eteq - could you have a look? Perhaps something that can be included in #3159.

@taldcroft
Copy link
Member

@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 Time and DateTime inputs (and other cases where the use of nditer was introduced).

@mhvk
Copy link
Contributor Author

mhvk commented Nov 30, 2014

Yes, I'll try to add some more scalar tests as well; for these, jd1 and jd2 are now stored as array scalar, and that already managed a few times to unintendedly percolate through.

@mhvk mhvk force-pushed the time-scalar-done-easy branch from 36dd832 to 870c238 Compare December 11, 2014 05:37
@mhvk mhvk added Affects-dev PRs and issues that do not impact an existing Astropy release Enhancement time labels Dec 11, 2014
@mhvk mhvk changed the title WIP Time scalar & multi-dimensional, using new ERFA Time scalar & multi-dimensional, using new ERFA Dec 11, 2014
@mhvk
Copy link
Contributor Author

mhvk commented Dec 11, 2014

OK, the multidimensional Time seems fairly complete now in tests as well. It may be good to wait for the erfa implementation to settle, though.

@mhvk mhvk force-pushed the time-scalar-done-easy branch from b287725 to afc1af8 Compare December 17, 2014 01:36
@mhvk
Copy link
Contributor Author

mhvk commented Dec 17, 2014

OK, after rebasing with #3166 in, this no longer depends on how erfa is done and should thus be ready for final review.

@taldcroft
Copy link
Member

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.

@mhvk
Copy link
Contributor Author

mhvk commented Dec 17, 2014

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 reshape, ravel, transpose, etc., but think that is better left for a later PR, as it needs some care with broadcasting the other array properties. (edit: issue #3224)

@mhvk
Copy link
Contributor Author

mhvk commented Dec 17, 2014

@taldcroft - do you want to have a quick look at the last commit with the documentation? I think this is ready to go in.

@mhvk mhvk mentioned this pull request Dec 17, 2014
@mhvk
Copy link
Contributor Author

mhvk commented Dec 17, 2014

@taldcroft - just in case: don't merge this yet -- waiting for #3173 to go in first...

@eteq
Copy link
Member

eteq commented Dec 17, 2014

@mhvk - #3173 is in so, rebase when ready (or #3173 - whichever order is easier on you)

@mhvk mhvk force-pushed the time-scalar-done-easy branch from 7fab31a to 59183e9 Compare December 18, 2014 00:04
@mhvk
Copy link
Contributor Author

mhvk commented Dec 18, 2014

@taldcroft - ok, now rebased again, and it should really be all OK. Maybe just have a quick look over the documentation.

@taldcroft
Copy link
Member

Docs look good, but it seems you need to rebase again.

CHANGES.rst Outdated
Copy link
Member

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.

@mhvk mhvk force-pushed the time-scalar-done-easy branch from 59183e9 to e915e3d Compare December 18, 2014 13:31
@astrofrog
Copy link
Member

👍 feel free to merge!

mhvk added a commit that referenced this pull request Dec 18, 2014
Time scalar & multi-dimensional, using new ERFA
@mhvk mhvk merged commit 72ca1bb into astropy:master Dec 18, 2014
@mhvk mhvk deleted the time-scalar-done-easy branch December 18, 2014 14:32
@embray embray added Affects-release and removed Ready-for-final-review Affects-dev PRs and issues that do not impact an existing Astropy release labels Dec 18, 2014
@embray
Copy link
Member

embray commented Dec 18, 2014

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".

@mhvk
Copy link
Contributor Author

mhvk commented Dec 18, 2014

OK, here I was thinking affects-release means it affects an already existing release.

@embray
Copy link
Member

embray commented Dec 18, 2014

In a way it does--it provides an enhancement over existing releases.

@embray
Copy link
Member

embray commented Dec 18, 2014

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.

taldcroft added a commit to taldcroft/astropy that referenced this pull request Dec 21, 2014
taldcroft added a commit to taldcroft/astropy that referenced this pull request Jan 10, 2015
taldcroft added a commit to taldcroft/astropy that referenced this pull request Jan 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants