Skip to content
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

[time] Make parse_time return astropy.time.Time #2611

Merged
merged 34 commits into from May 22, 2018

Conversation

Projects
None yet
4 participants
@vn-ki
Copy link
Member

commented May 2, 2018

parse_time now returns Time. The test for parse_time are fixed too.


Notes to reviewers

This PR is against the "time" branch of SunPy, where we will be doing incremental PRs as we refactor the whole code base to use Astropy Time. The plan is to review each PR to the time branch in detail, so the massive merge PR dosen't require a massive review at the end.

This PR is (hopefully) the only massive API change (beyond everything returning Time), so it's important we get this right. Also the tests will fail until we have completed the migration across much more of the code.

Here is a summary of the API:

Strings

parse_time call: parse_time('2005-08-04T00:18:02.000', scale='tt')
Parsed Time: <Time object: scale='tt' format='isot' value=2005-08-04T00:18:02.000>

parse_time call: parse_time('20140101000001')
Parsed Time: <Time object: scale='utc' format='isot' value=2014-01-01T00:00:01.000>

parse_time call: parse_time('2016.05.04_21:08:12_TAI')
Parsed Time: <Time object: scale='tai' format='isot' value=2016-05-04T21:08:12.000>

parse_time call: parse_time('1995-12-31 23:59:60')
Parsed Time: <Time object: scale='utc' format='isot' value=1995-12-31T23:59:60.000>

parse_time call: parse_time('1995-Dec-31 23:59:60')
Parsed Time: <Time object: scale='utc' format='isot' value=1995-12-31T23:59:60.000>

Datetime

parse_time call: parse_time(datetime.datetime(2018, 5, 13, 15, 46, 26, 317773), scale='tai')
Parsed Time: <Time object: scale='tai' format='datetime' value=2018-05-13 15:46:26.317773>

parse_time call: parse_time(datetime.date(2018, 5, 13))
Parsed Time: <Time object: scale='utc' format='iso' value=2018-05-13 00:00:00.000>

numpy.datetime64

parse_time call: parse_time(numpy.datetime64('1995-12-31T23:59:59'))
Parsed Time: <Time object: scale='utc' format='isot' value=1995-12-31T23:59:59.000>

parse_time call: parse_time(array(['2005-02-01T00', '2005-02-01T01', '2005-02-01T02', '2005-02-01T03',
       '2005-02-01T04', '2005-02-01T05', '2005-02-01T06', '2005-02-01T07',
       '2005-02-01T08', '2005-02-01T09'], dtype='datetime64[h]'))
Parsed Time: <Time object: scale='utc' format='isot' value=['2005-02-01T00:00:00.000' '2005-02-01T01:00:00.000'
 '2005-02-01T02:00:00.000' '2005-02-01T03:00:00.000'
 '2005-02-01T04:00:00.000' '2005-02-01T05:00:00.000'
 '2005-02-01T06:00:00.000' '2005-02-01T07:00:00.000'
 '2005-02-01T08:00:00.000' '2005-02-01T09:00:00.000']>

AstroPy compatible

parse_time call: parse_time(1234.0, format=jd)
Parsed Time: <Time object: scale='utc' format='jd' value=1234.0>

parse_time call: parse_time('B1950.0', format=byear_str)
Parsed Time: <Time object: scale='utc' format='byear_str' value=B1950.000>

parse_time call: parse_time('2001-03-22 00:01:44.732327132980', scale='utc', location=('120d', '40d'))
Parsed Time: <Time object: scale='utc' format='iso' value=2001-03-22 00:01:44.732>

Pandas

parse_time call: parse_time(pandas.Timestamp('1966-02-03 00:00:00'))
Parsed Time: <Time object: scale='utc' format='datetime' value=1966-02-03 00:00:00>

parse_time call: parse_time(pandas.Series([[2012-01-01 00:00:00, 2012-01-02 00:00:00]
                                           [2012-01-03 00:00:00, 2012-01-04 00:00:00]]))
Parsed Time: <Time object: scale='utc' format='datetime' value=[[datetime.datetime(2012, 1, 1, 0, 0) datetime.datetime(2012, 1, 2, 0, 0)]
 [datetime.datetime(2012, 1, 3, 0, 0) datetime.datetime(2012, 1, 4, 0, 0)]]>
@pep8speaks

This comment has been minimized.

Copy link

commented May 2, 2018

Hello @vn-ki! Thanks for updating the PR.

Line 48:25: E128 continuation line under-indented for visual indent
Line 48:101: E501 line too long (103 > 100 characters)
Line 88:101: E501 line too long (101 > 100 characters)
Line 89:33: E128 continuation line under-indented for visual indent

Line 401:1: E303 too many blank lines (3)

Comment last updated on May 16, 2018 at 15:50 Hours UTC

dt64 = np.datetime64('2014-02-07T16:47:51.008288123')
dt = parse_time(dt64)

assert dt == datetime(2014, 2, 7, 16, 47, 51, 8288)
assert dt == ap.Time('2014-02-07T16:47:51.008288123', format='isot')


def test_parse_time_numpy_datetime_round():

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 4, 2018

Author Member

Should the name be changed since it's not rounding anymore?

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

yeah I think so



def test_parse_time_pandas_timestamp():
ts = pandas.Timestamp(LANDING)
ts = pandas.Timestamp(datetime(*(1966, 2, 3)))

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 4, 2018

Author Member

datetime(1996, 2, 3)



@convert_time.register(date)
def convert_time_date(time_string, **kwargs):
return datetime.combine(time_string, time())
return ap.Time(datetime.combine(time_string, time()))

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 5, 2018

Author Member

Use ap.Time(time_string.isoformat())?

@vn-ki vn-ki force-pushed the vn-ki:astropy-time branch from 3564f26 to 308fa1e May 5, 2018

@Cadair
Copy link
Member

left a comment

Didn't quite make it all the way through before I had to get on a plane but it is looking great.

@@ -5,3 +5,4 @@
from sunpy.time.timerange import *
from sunpy.time.julian import *
from sunpy.time.utime import *
from sunpy.time import astropy_time

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

I would change this to from sunpy.time.astropy_time import Time so you can just from sunpy.time import Time and then when we retire the subclass it's just a s/sunpy.time/astropy.time


from time import strftime, strptime
from datetime import date, datetime

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

This needs an __all__ list.

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

done

from datetime import date, datetime


class Time(astropy.time.Time):

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

When this is merged into astropy we should attempt to use the one in astropy if it exists. i.e.

if hasattr(astropy.time.Time, strptime):
    from astropy.time import TIme
else:
    class Time(...

dt64 = np.datetime64('2014-02-07T16:47:51.008288123')
dt = parse_time(dt64)

assert dt == datetime(2014, 2, 7, 16, 47, 51, 8288)
assert dt == ap.Time('2014-02-07T16:47:51.008288123', format='isot')


def test_parse_time_numpy_datetime_round():

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

yeah I think so

@@ -79,16 +82,16 @@ def _regex_parse_time(inp, format):
try:
hour = match.group("hour")
except IndexError:
return inp, timedelta(days=0)
return inp, astropy.time.TimeDelta(u.day*0)

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

0*u.day just because I am a pendant lol

@@ -142,67 +137,64 @@ def convert_time(time_string, **kwargs):

@convert_time.register(pandas.Timestamp)
def convert_time_pandasTimestamp(time_string, **kwargs):
return time_string.to_pydatetime()
return ap.Time(time_string)

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

I am impressed this works.

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 5, 2018

Author Member

I was too. pandas.Timestamp inherits datetime. So isinstance checks out. And it works.

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 5, 2018

Author Member

Just to be safe, maybe we should do ap.Time(time_string.to_pydatetime()).



@convert_time.register(datetime)
def convert_time_datetime(time_string, **kwargs):
return time_string
return ap.Time(time_string)

This comment has been minimized.

Copy link
@Cadair

Cadair May 5, 2018

Member

I see that there are a few of these dispatch functions that are just ap.Time(time_string) we should just define this once and have multiple decorators on it, i.e.

@convert_time.register(datetime)
@convert_time.register(pandas.Timestamp)
@convert_time.register(astropy.time.Time)
 def convert_time_constructor(time_string, **kwargs):
    return ap.Time(time_string)
@vn-ki

This comment has been minimized.

Copy link
Member Author

commented May 6, 2018

@Cadair @nabobalis @Punyaslok

What should we do about time_format?
Maybe it can be passed to Time as format while handling the default.

def convert_time(time_string, time_format=None, **kwargs):
    # default case when no type matches
    try:
        Time(time_string, format=time_format, **kwargs)
    except ValueError:
        raise ValueError("'{tstr!s}' is not a valid time string!".format(tstr=time_string))



def parse_time(..., time_format=None, ...):
    if time_format:
        convert_time.dispatch(object)(time_string, time_format, **kwargs)

This means that if SunPy can't parse it, it will fallback to Astropy parsing and it will fix #2155.

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

Can we not call it just format?

@vn-ki

This comment has been minimized.

Copy link
Member Author

commented May 8, 2018

There is an out_format. So best if we call it time_format or in_format.

@Cadair Cadair changed the base branch from master to time May 9, 2018

@convert_time.register(datetime)
def convert_time_datetime(time_string, **kwargs):
return time_string
return ap.Time(time_string)

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

I think that all the instances of ap.Time(.... here should take the **kwargs so things from the parse_time api get passed into Time

@vn-ki vn-ki force-pushed the vn-ki:astropy-time branch from 87135d4 to 40f3433 May 9, 2018

@convert_time.register(pandas.Timestamp)
def convert_time_pandasTimestamp(time_string, **kwargs):
return time_string.to_pydatetime()
try:

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

Do we need this try block? can we not just delegate to astropy Time to raise an error if it does not understand the input?



@convert_time.register(float)
@convert_time.register(int)
def convert_time_float(time_string, **kwargs):
return datetime(1979, 1, 1) + timedelta(0, time_string)
return ap.Time(time_string, format='utime', **kwargs)

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

I don't think we need this anymore. utime is not the only valid float input to Time and you have to specify format='utime' if you want to provide parse_time with a utime, so this should just be handled by the default case.

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 9, 2018

Author Member

A number of Time formats have float and int input. jyear, jd, gps, decimalyear etc.

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

exactly, so registering this for all float inputs is wrong. We should just hand this off to the Time constructor.

@@ -251,12 +245,14 @@ def parse_time(time_string, time_format='', **kwargs):
>>> sunpy.time.parse_time('2005-08-04T00:01:02.000Z')
datetime.datetime(2005, 8, 4, 0, 1, 2)
"""
if time_format == 'utime':
return convert_time(float(time_string), **kwargs)
if format:

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

I don't think you need the if here.You should just be able to do this as:

if time_string is 'now':
    time_string = ap.Time.now()

rt = convert_time.dispatch(object)(time_string, format, **kwargs)
@convert_time.register(int)
def convert_time_float(time_string, **kwargs):
return datetime(1979, 1, 1) + timedelta(0, time_string)
return ap.Time('{}-{}-{}'.format(*time_string), **kwargs)

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

what about tuples that also include times? i.e (2011, 1, 1, 10, 12, 12)?

else:
return convert_time.dispatch(object)(time_string, **kwargs)


@convert_time.register(astropy.time.Time)
def convert_time_astropy(time_string, **kwargs):
return time_string.datetime
return time_string

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

Do we need this? Isn't this handled by the default case (i.e. you can pass an astropy time to Time()

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 9, 2018

Author Member

It was throwing an error about a missing _time while checking equality. I have to look into it.

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 14, 2018

Author Member

Astropy bug: astropy/astropy#7449

This comment has been minimized.

Copy link
@Cadair

Cadair May 22, 2018

Member

We should add a TODO here about dropping that once we can rely on the bug fix in astropy

@@ -212,22 +188,25 @@ def convert_time_str(time_string, **kwargs):
break
if ts is None:
continue
return datetime.strptime(ts, time_format) + time_delta
return ap.Time.strptime(ts, time_format,
**kwargs) + time_delta
except ValueError:
pass
time_string_parse_format = kwargs.pop('_time_string_parse_format', None)

This comment has been minimized.

Copy link
@Cadair

Cadair May 9, 2018

Member

This is probably best done as a followup PR, but I think I would like to remove this functionality from parse_time.



def test_parse_time_astropy_formats():
lst = [

This comment has been minimized.



def test_parse_time_numpy_datetime():
inputs = np.arange('2005-02-01T00', '2005-02-01T10', dtype='datetime64')

dts = parse_time(inputs)

assert isinstance(dts, np.ndarray)
assert all([isinstance(dt, datetime) for dt in dts])
assert isinstance(dts, astropy.time.Time)

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 14, 2018

Author Member

Check equality

assert parse_time(k) == v
dt = parse_time(k)
assert dt == v
dt.format == 'isot'

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 14, 2018

Author Member

Missing assert


assert dt1.jd == dt2.jd


This comment has been minimized.

Copy link
@vn-ki

vn-ki May 14, 2018

Author Member

Parse astropy incompatible string


from . import astropy_time as ap

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 16, 2018

Author Member

You want this to be an absolute import?

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

I am for absolute import.

"""
================================================
Parsing times with sunpy.time.parse_time
================================================

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

I forget if Github uses a monospace font but the = should be the same length as the title.

Parsing times with sunpy.time.parse_time
================================================
Example to show some example usage of parse_time

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

I would replace with line with something a bit more verbose.

Example to show some example usage of parse_time
"""
##############################################################################
# Import the required modules

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

Full stop missing at the end.

import numpy as np
import pandas

# Import parse_time

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

not sure we need this comment here.



##############################################################################
# Suppose you want to parse some strings, ``parse_time`` can do that.

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

Does intersphinx work here? I forget if its single backticks.

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 16, 2018

Author Member

@Cadair said do "``"

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

I do forget but I thought just single ones work.

This comment has been minimized.

Copy link
@Cadair

Cadair May 16, 2018

Member

you will need ~sunpy.time.parse_time but it should work.

t1 = parse_time('1995-12-31 23:59:60')

##############################################################################
# Of course you could do the same with astropy ``Time``.

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

Should intersphinx to Astropy Time here.

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 16, 2018

Author Member

Inter sphinx works as ~astropy.time.Time, right?

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

Yeah, that should work.

##############################################################################
# Of course you could do the same with astropy ``Time``.
# But SunPy ``parse_time`` can parse even more formats of time strings.
# And as you see from the examples, thanks to astropy ``Time``, ``parse_time``

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

Here too for both lines.

time_obj1 = Time('1995-12-31T23:59:60', format='isot')
time_obj2 = Time.strptime('1995-Dec-31 23:59:60', '%Y-%b-%d %H:%M:%S')

assert time_obj1 == time_obj2

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

So let me just be certain/correct.

This is the Astropy code that in the open PRs which are here until they get merged in?

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 16, 2018

Author Member

Yes. Astropy code and astropy tests

format : [ 'jd', 'mjd', 'decimalyear', 'unix', 'cxcsec', 'gps',
'plot_date', 'datetime', 'iso', 'isot', 'yday', 'fits',
'byear', 'jyear', 'byear_str', 'jyear_str', 'utime']

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

Should we not link to astropy docs here?

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 16, 2018

Author Member

'utime' is not in the docs. It is a sunpy only custom format. I can link the docs if you want.

This comment has been minimized.

Copy link
@nabobalis

nabobalis May 16, 2018

Contributor

Well It's just if that we support astropy time formats + utime formats, no need to duplicate it fully here.

This comment has been minimized.

Copy link
@vn-ki

vn-ki May 16, 2018

Author Member

Can I put a link in the format part? Should I put it in a separate note at the end?

@nabobalis
Copy link
Contributor

left a comment

I can't see anything I have a problem with.

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

I think that maybe the example should be moved into the docs fully as a page for handling time.

@Cadair

This comment has been minimized.

Copy link
Member

commented May 16, 2018

I think it's probably worth adding a section on Time in general, probably worth doing once the majority of the migration is complete?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

Yeah thats a better idea.

vn-ki added some commits May 16, 2018



def parse_time(time_string, time_format='', **kwargs):
def parse_time(time_string, format=None, **kwargs):

This comment has been minimized.

Copy link
@Cadair

Cadair May 16, 2018

Member
def parse_time(time_string, *, format=None, **kwargs)
@Cadair

Cadair approved these changes May 22, 2018

@Cadair Cadair merged commit a33c228 into sunpy:time May 22, 2018

3 of 7 checks passed

ci/circleci: astropy-time Your tests failed on CircleCI
Details
ci/circleci: html-docs Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci: egg-info-27 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-35 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details

@Cadair Cadair modified the milestones: 1.0, Time May 28, 2018

@vn-ki vn-ki deleted the vn-ki:astropy-time branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.