Allow Time input/interaction/output with Quantities (closes #751) #1431

mhvk · 2013-09-10T05:32:18Z

In #751, it was argued that Time and Quantity should interact properly, with @taldcroft giving a list of expected behavour:

One should be able to convert TimeDelta to different units with to().
Adding or subtracting Quantity and TimeDelta or Time (regardless of argument order) should give a TimeDelta.
Multiplying or dividing a TimeDelta by a Quantity should result in a Quantity.
Multiplying or dividing a DeltaTime by a scalar should result in a DeltaTime.

Furthermore, currently, input of Quantities is ignored -- just the value is taken (TimeDelta(10.*u.s, format='jd') silently gives a 10-day time difference; same for Time).

This PR addresses the initialisation of both Time and TimeDelta and implements most of the above -- the exception is that Quantity <OP> TimeDelta does not work, as Quantity (and ndarray) fail to return NotImplemented on trying to add a Time or TimeDelta [EDIT: now raised an issue, #1455])
Suggestions most welcome; if it is agreed this is in the right direction, I can add tests/documentation (PR's to my branch more than welcome...).

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

In [2]: t1=Time('2012-01-01', scale='utc')

In [3]: t1 + 1*u.d
Out[3]: <Time object: scale='utc' format='iso' value=2012-01-02 00:00:00.000>

In [4]: t1 + 1*u.yr
Out[4]: <Time object: scale='utc' format='iso' value=2012-12-31 05:59:59.000>
# u.yr = Julian year = 365.25 day, but there was a leap second and a leap day

In [5]: dt = Time('2013-01-01', scale='utc')-t1

In [6]: dt + 1.*u.d
Out[6]: <TimeDelta object: scale='tai' format='jd' value=367.000011574>

In [7]: dt * (10. * u.m / u.s**2)
Out[7]: <Quantity 3660.00011574 d m / s2>

In [8]: 10.*u.cycle / dt
ERROR: TypeError: scalar 'TimeDelta' object is not subscriptable. [astropy.time.core]
...   # Quantity should return NotImplemented so that TimeDelta can have a go
In [9]: 10. / dt
Out[9]: <Quantity 0.0273224035076 1 / d>

In [10]: dt.to(u.yr)
Out[10]: <Quantity 1.00205341978 yr>

mdboom · 2013-09-10T12:22:43Z

astropy/time/core.py

Maybe this should be other.__sub__(self) -- could make a difference if the datatype is unsigned (I know that would be bad in other ways, but why not?)

@mdboom - no, it has to call itself: the reason we can get here is if, e.g., one does Quantity - TimeDelta. For this case, python first gives Quantity a chance, looking up its __sub__ method. It should raise NotImplemented (it doesn't yet, but it should), as it doesn't know how to add to a TimeDelta. Then, python checks whether TimeDelta has a __rsub__ method, and gives that a chance: but of course this then should use its own method, as for this case we know other.__sub__ fails.

mhvk · 2013-09-10T14:07:06Z

@mdboom - only one of your comments had a very clear answer -- the others all bring up in one way or another a more general question I was thinking about while implementing this: to what extent should Time start to depend on quantities? (@taldcroft may also want to pipe in).

Here, a range of options are available:

Right now, I opted to try to avoid importing astropy.units, rather just aiming to duck type, i.e., try whether something behaves like a quantity. An advantage is that at least in principle Time can still be pulled out of astropy and used on its own. But it meant that, e.g., I do things like other.to('day') rather than the u.Unit(getattr(other, 'unit', 1.), u.day) you suggested above.
I could import units locally, perhaps still hidden behind an if getattr(other, "to") statement that checks whether something at least potentially is a quantity.
Things would be somewhat easier if I just imported astropy.units on top. I was a bit wary about this, as for the sidereal time effort in Sidereal time #1418, I found I could not import from coordinates directly (since that imports Time, causing circular dependencies). But I just tried and can import units without problem (not surprisingly, really).
Rather than try to convert quantities to floats with units of day, might it be easier to use quantities, converting floats to a standard unit where necessary; i.e., should jd1 and jd2 be quantities with units of days? It would also, e.g., allow one to use unit as a proper Unit instead of a numerical scale factor.

Of course, these are mostly design choices and I'm not quite clear what the astropy "ideals" are in this respect. E.g., I noticed in various pieces of code that external routines are only imported locally (a procedure I followed for iers), but I'm not quite clear when that should or should not be done. A clear disadvantage would seem to be that one cannot tell from the top of the file any more what a routine really depends on.

mhvk · 2013-09-15T05:00:53Z

@mbboom, @taldcroft - I added lots of test cases and rebased. But my questions above still hold...

Also, to avoid it disappearing in an outdated diff: @mboom, you asked

Would it be helpful to define Besselian year as a unit? It wouldn't be convertible to other years that way (because it requires an offset and unit conversions don't support that), but it might provide a consistent way to instantiate these things.

I'm not sure -- it seems a bit overkill, and I'm not sure how useful it is if it is not convertible (and I do not see how to convert it).

mhvk · 2013-09-16T20:20:02Z

Just tried and failed to understand what the test failure was for python 3.3 -- suggestions welcome...
EDIT -- this has disappeared

taldcroft · 2013-09-17T16:17:45Z

@mhvk - this and some of your other recent PRs are on my radar, but my day job is getting in the way big time. Apologies and thanks for your patience...

mhvk · 2013-09-18T03:11:34Z

@mdboom, @taldcroft - I noticed that the test cases I added actually hadn't made it in -- the risks of git commit -a... Now they are in; it gives perhaps a better sense of what now is possible.

mhvk · 2013-09-21T02:07:54Z

The travis build error is from the documentation, with a complaint that the right version of sphinx is not available.

astrofrog · 2013-09-21T02:20:45Z

Ah right, I've opened a PR with a fix: #1478

taldcroft · 2013-09-22T22:40:33Z

@mhvk - This is really great and a very good addition to Time. Thanks for the hard work!

It looks like there is still a test failure for python 3.3 that needs fixing. Otherwise what would be good is a section in the Time docs like Time and Quantities that gives examples like in your tests of how to use Quantities in Time. I think it should be the last subsection of Using Time objects after Time Deltas.

astrofrog · 2013-10-05T19:45:05Z

@mhvk - you will need to rebase to get rid of the Sphinx failure.

mhvk · 2013-10-05T21:21:29Z

@astrofrog - OK, rebased. Still to do are @taldcroft's suggested updates to the documentation.

mhvk · 2013-10-07T04:23:51Z

@taldcroft - now updated the documentation. Maybe we can chat tomorrow about the more general questions I posed earlier (about where to import units, etc.)

mhvk · 2013-10-14T03:27:48Z

@taldcroft, @mdboom, @astrofrog - this seems quite complete; just pinging in case it was being forgotten...

astrofrog · 2013-10-14T06:38:09Z

@mhvk - there are a couple of warnings in the docs causing the failure:

/home/travis/build/astropy/astropy/docs/time/index.rst:618: ERROR: Undefined substitution referenced: "Quantity".
/home/travis/build/astropy/astropy/docs/time/index.rst:618: ERROR: Undefined substitution referenced: "Quantity".

I'll leave it up to @taldcroft to do a final review and merge.

mhvk · 2013-10-14T17:31:08Z

@astrofrog - I made a correction, but travis seems to have balked for reasons that appear to be unrelated to this.

astrofrog · 2013-10-14T18:49:03Z

I've restarted Travis

taldcroft · 2013-10-14T22:10:37Z

@mhvk - I was (and am) waiting on a clean Travis doc build before merge, otherwise this looks fine (no, great!). The doc build error is occurring in astropy.time code so it may indeed be related. Are you able to build the docs locally?

mhvk · 2013-10-15T01:34:07Z

@taldcroft - I finally moved over to python3 and thus could check the build failure -- it was an AttributeError in a condition that was meant to fail (but differently, not completely clear why 3.3 is different). To avoid this recurring, I changed some if not-good: raise to try: assert-good, except: raise; this seems safer and a bit more pythonesque.

Now if only travis would start, we'd know if this is OK (@astrofrog?)...

astrofrog · 2013-10-15T06:18:05Z

@taldcroft - tests are passing, feel free to merge.

Allow Time input/interaction/output with Quantities (closes #751)

mdboom reviewed Sep 10, 2013
View reviewed changes

taldcroft mentioned this pull request Sep 20, 2013

Make TimeDelta a unitful Quantity #751

Closed

mhvk added 8 commits October 14, 2013 21:52

Allow input/interaction/output with Quantities (closes astropy#751)

ab94a2e

Remove print statements left-over from testing

4d79bec

Add test cases, resolve some problems that appeared

55eaa73

Add tests for time/quantity interaction

7faf215

Also define __rtruediv__ so python3 works correctly

5a493f7

Add documentation for interaction of Time and TimeDelta with Quantity

3fb9708

Correct documentation for unresolvable links to Quantity

cc6f999

Rewrite of value validation with try/except to avoid python3 problem

7a05af8

taldcroft added a commit that referenced this pull request Oct 15, 2013

Merge pull request #1431 from mhvk/time-accept-quantity

04c7598

Allow Time input/interaction/output with Quantities (closes #751)

taldcroft merged commit 04c7598 into astropy:master Oct 15, 2013

mhvk deleted the time-accept-quantity branch October 15, 2013 13:13

mhvk mentioned this pull request Oct 15, 2013

Trial: Table Column subclassed from quantity #1583

Closed

taldcroft added a commit that referenced this pull request Oct 21, 2013

Update CHANGES.rst for #1431 (Time and Quantity interaction)

00f6076

Uh oh!

Allow Time input/interaction/output with Quantities (closes #751) #1431

Allow Time input/interaction/output with Quantities (closes #751) #1431

Uh oh!

Conversation

mhvk commented Sep 10, 2013

Uh oh!

mdboom Sep 10, 2013

Choose a reason for hiding this comment

Uh oh!

mhvk Sep 10, 2013

Choose a reason for hiding this comment

Uh oh!

mhvk commented Sep 10, 2013

Uh oh!

mhvk commented Sep 15, 2013

Uh oh!

mhvk commented Sep 16, 2013

Uh oh!

taldcroft commented Sep 17, 2013

Uh oh!

mhvk commented Sep 18, 2013

Uh oh!

mhvk commented Sep 21, 2013

Uh oh!

astrofrog commented Sep 21, 2013

Uh oh!

taldcroft commented Sep 22, 2013

Uh oh!

astrofrog commented Oct 5, 2013

Uh oh!

mhvk commented Oct 5, 2013

Uh oh!

mhvk commented Oct 7, 2013

Uh oh!

mhvk commented Oct 14, 2013

Uh oh!

astrofrog commented Oct 14, 2013

Uh oh!

mhvk commented Oct 14, 2013

Uh oh!

astrofrog commented Oct 14, 2013

Uh oh!

taldcroft commented Oct 14, 2013

Uh oh!

mhvk commented Oct 15, 2013

Uh oh!

astrofrog commented Oct 15, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants