[Review]: Clean up `parse_time` API #887

gunner272 · 2014-03-03T19:40:28Z

I added the small ability to parse international time seconds(time elapsed in seconds from 1958,1,1) to time.parse_time function ,Please provide feedback

Thank You

derdon · 2014-03-03T20:16:42Z

I have a déjà vu …

gunner272 · 2014-03-03T20:27:55Z

I closed the previous request
#870
Opened a new one ,with the same changes but removing the unwarranted ones

derdon · 2014-03-03T20:40:09Z

sunpy/time/time.py

    """
    if isinstance(time_string, datetime):
        return time_string
    elif isinstance(time_string, tuple):
        return datetime(*time_string)
+    elif tai and ( isinstance( time_string, int) or isinstance( time_string, float)):


fix the formatting to: elif tai and isinstance(time_string, int) or isinstance(time_string, float):

Doing the above was clashing with other unit test cases

def test_parse_time_int(): > assert parse_time(765548612.0) == datetime(2003, 4, 5, 12, 23, 32) E assert datetime.datetime(1982, 4, 5, 12, 23, 32) == datetime.datetime(2003, 4, 5, 12, 23, 32) E + where datetime.datetime(1982, 4, 5, 12, 23, 32) = parse_time(765548612.0) E + and datetime.datetime(2003, 4, 5, 12, 23, 32) = datetime(2003, 4, 5, 12, 23, 32) time/tests/test_time.py:24: AssertionError

coveralls · 2014-03-03T20:54:01Z

Coverage increased (+0.02%) when pulling 4ca1f61 on gunner272:tai into 76ae390 on sunpy:master.

coveralls · 2014-03-03T21:34:40Z

Coverage increased (+0.01%) when pulling 4ea62c6 on gunner272:tai into 76ae390 on sunpy:master.

dpshelio · 2014-03-05T06:28:19Z

sunpy/time/time.py

    """
    if isinstance(time_string, datetime):
        return time_string
    elif isinstance(time_string, tuple):
        return datetime(*time_string)
+    elif tai and  isinstance( time_string, int) or tai and isinstance( time_string, float) :


A question to the most pythonists! Would it be consider a bad practice to put this as:

elif (tai and isinstance( time_string, int)) or (tai and isinstance (time_string, float)):

? This makes it, at least to me, easier to read.

But, in any case, could we not just do this as:

elif tai and (isinstance( time_string, int)) or isinstance (time_string, float)):

I believe in this case the () are essential, otherwise we could get True by just having time_string as float.

and has a higher priority than or. The syntax of Python is defined in a way that (expr) is the same as expr, the parentheses only determine the precedence level. (those two things are very common in all languages I know that contain some form of and/&& and parentheses to denote the precedence level). Another important thing to know is that for boolean algebra the distributive law holds: A AND (B OR C) = A AND B OR A AND C.

To answer your question @dpshelio: keeping these facts in mind, the first example tai and isinstance(time_string, int) or tai and isinstance(time_string, float) (I removed unnecessary parens) is equivalent to tai and (isinstance(time_string, int) or isinstance(time_string, float))

gunner272 · 2014-03-05T20:00:18Z

I agree with both of you ,from a personal opinion I prefer the style with the parenthesis,gives an easier to understand code

derdon · 2014-03-05T21:51:24Z

I didn't share my opinion. I just explained which expressions are equivalent. Now to share my opinion: with only two extra expressions (those that check the type) I don't have any strong feelings. But especially if it gets more, I'm in favor of the solution which is of the form tai and (A or B or C or …) because it obeys DRY (don't repeat yourself) and makes the code more extensible and maintainable.

gunner272 · 2014-03-06T14:39:56Z

I am a bit confused,should I change it back?

derdon · 2014-03-06T15:21:55Z

Well, at least you should follow PEP 8 and also remove unnecessary parens. The other thing is subjective for such a short line.

dpshelio · 2014-03-09T09:52:42Z

Thanks @derdon for the explanation and your point of view. I also prefer the:

tai and (A or B or C ...)

approach.

@gunner272, could you make the changes suggested in the docstring and change the comparison from above obeying DRY.

dpshelio · 2014-03-09T09:54:37Z

sunpy/time/tests/test_time.py

@@ -23,6 +23,16 @@ def test_parse_time_tuple():
 def test_parse_time_int():
    assert parse_time(765548612.0) == datetime(2003, 4, 5, 12, 23, 32)
    assert parse_time(1009685652.0) == datetime(2010, 12, 30, 4, 14, 12)
+
+def test_parse_time_tai():


Also, could you insert a commented line or docstring in here pointing us to the source you've used to get the TAI values? webpage, algorithm, etc..

I am not sure about the resource because these values were manually calculated by me,I believe they would be correct ,but please feel free to cross check the calculations

gunner272 · 2014-03-09T13:31:55Z

@dpshelio ,Could you please put a line note on the docstring I should edit

dpshelio · 2014-03-10T16:22:24Z

@gunner272 I think is failing because the GOES api gone by the wind....

aringlis · 2014-04-30T14:06:57Z

What's the status of this, now that the GOES lightcurve source has been fixed by #942?

Cadair · 2014-05-12T16:24:49Z

Can you change this so that it would look more like this:

parse_time(time, format='tai')

can you also add format='utime' which I think is the current default. Also can someone please suggest a docstring that explains what the hell utime actually is?!

like this:
http://docs.astropy.org/en/stable/time/index.html

aringlis · 2014-05-12T16:27:08Z

Copied from 'anytim.pro': UTIME - Utime format, Real*8 seconds since 1-jan-79

Cadair · 2014-05-12T16:29:37Z

oohhhhh solarsoft ... bite me

aringlis · 2014-05-19T16:30:13Z

👍

ayshih · 2014-05-19T16:32:54Z

~~This PR will be blocked until an outstanding issue with leap seconds is resolved.~~

Now resolved

Cadair · 2014-05-19T16:33:25Z

sunpy/time/time.py


-    .. todo::
-        add ability to parse tai (International Atomic Time seconds since
-        Jan 1, 1958)
    """
    if isinstance(time_string, datetime):
        return time_string
    elif isinstance(time_string, tuple):


this would be better if it tested for all iterables that are not strings rather than just tuples.

there is a is_iterable function in astropy somewhere.

ehsteve · 2014-05-19T16:37:11Z

Btw, astropy.time does seem to handle leap seconds. See http://astropy.readthedocs.org/en/latest/time/.

Cadair · 2014-06-02T11:06:53Z

@gunner272 Something has upset things. Looks like the API has changed in a non-backwards compatible way.

specifically I think that it no longer assumes 'utime' if a float is passed in. (https://travis-ci.org/sunpy/sunpy/jobs/26442840#L1351)

I suggest two things,

I like that it dosen't understand floats by default, so the best thing to do is fix the places where parse_time is called for utime.
Make the error nicer if a float is passed when the time_format is a string.

gunner272 · 2014-06-02T11:40:15Z

@Cadair : like the change above if isinstance(time_string, datetime) or time_format == 'datetime':
shouldn't this also be changed to
time_format == 'utime' or ( isinstance(time_string, int) or isinstance(time_string, float) )

Cadair · 2014-06-02T11:43:37Z

@gunner272 that is the simple solution, that doesn't change the API. I suppose for now that is the best bet.

Can you make sure that behaviour is documented, that it assumes floats are 'utime'?

coveralls · 2014-06-02T20:03:58Z

Coverage increased (+0.64%) when pulling ee5bb21 on gunner272:tai into b37e155 on sunpy:master.

derdon · 2014-06-02T20:10:25Z

sunpy/time/time.py

        return time_string
    elif isinstance(time_string, tuple):
        return datetime(*time_string)
-    elif isinstance(time_string, int) or isinstance(time_string, float):
+    elif time_format == 'utime' or ( isinstance(time_string, int) or isinstance(time_string, float) ) :


elif time_format == 'utime' or isinstance(time_string, (int, float)): does the same.

ehsteve · 2014-06-09T13:10:21Z

sunpy/time/time.py

-
+    time_string : [ int, float, time_string, datetime ]
+        Date to parse which can be either time_string, int, datetime object.
+    format : [ basestring, utime, datetime ]


this should be time_format, right? This doc should also define each of the possibilities (such as what is utime).

Cadair · 2014-07-14T11:18:58Z

sunpy/time/time.py

-
+
+    Note:
+    If time_string is an instance of float, then it is assumed to be in unix time format.


this should be "assumed to be utime" right?

Cadair · 2014-07-14T11:39:02Z

👍

dpshelio · 2014-07-14T16:16:19Z

sunpy/time/time.py

@@ -256,18 +260,15 @@ def day_of_year(time_string):
    time_diff = time - datetime(time.year, 1, 1, 0, 0, 0)
    return time_diff.days + time_diff.seconds / SECONDS_IN_DAY + 1

-
-def break_time(t='now'):
+def break_time(t=None, time_format=''):


Should this None not be 'now'?

yes it should, good spot.

aringlis · 2014-07-14T16:46:42Z

👍

coveralls · 2014-07-15T12:46:09Z

Coverage decreased (-0.1%) when pulling 40c2f19 on gunner272:tai into 57ba719 on sunpy:master.

Cadair · 2014-08-04T16:18:00Z

@gunner272 you have a doc fail: https://travis-ci.org/sunpy/sunpy/jobs/29926813#L1485

ehsteve · 2014-08-25T16:29:39Z

👍

[Review]: Clean up `parse_time` API

Added TAI parsing ability time.parse_time

4ca1f61

derdon reviewed Mar 3, 2014
View reviewed changes

Reformmatting changes

4ea62c6

dpshelio reviewed Mar 5, 2014
View reviewed changes

dpshelio reviewed Mar 9, 2014
View reviewed changes

Edited docstring of parse_time parameters

49965e1

lets see

295aa9c

gunner272 added 2 commits May 14, 2014 10:15

time_format added to parse_time

ef2d29f

parse_time parameter time_format added

872358e

Cadair reviewed May 19, 2014
View reviewed changes

Minor change

b07dc90

float interpreted as utime

ee5bb21

derdon reviewed Jun 2, 2014
View reviewed changes

Cadair modified the milestone: 0.5 Jun 6, 2014

Cadair added the [Review] label Jun 6, 2014

Cadair changed the title ~~Clean up parse_time API~~ [Review]: Clean up parse_time API Jun 6, 2014

ehsteve reviewed Jun 9, 2014
View reviewed changes

Cadair removed this from the 0.5 milestone Jun 12, 2014

gunner272 added 2 commits June 22, 2014 13:14

changes incorporated

e898434

merge conflicts resolved

af4b672

Cadair reviewed Jul 14, 2014
View reviewed changes

Docstring corrected

77b7406

dpshelio reviewed Jul 14, 2014
View reviewed changes

break_time method parameter defaults to 'now' from now on

40c2f19

merge conflict removal

fb2d014

ehsteve added a commit that referenced this pull request Aug 25, 2014

Merge pull request #887 from gunner272/tai

806ba36

[Review]: Clean up `parse_time` API

ehsteve merged commit 806ba36 into sunpy:master Aug 25, 2014

dpshelio removed the Needs Review Needs reviews before merge label Apr 23, 2015



		Note:
		If time_string is an instance of float, then it is assumed to be in unix time format.

[Review]: Clean up parse_time API #887

[Review]: Clean up parse_time API #887

Conversation

gunner272 commented Mar 3, 2014

derdon commented Mar 3, 2014

gunner272 commented Mar 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 3, 2014

coveralls commented Mar 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunner272 commented Mar 5, 2014

derdon commented Mar 5, 2014

gunner272 commented Mar 6, 2014

derdon commented Mar 6, 2014

dpshelio commented Mar 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunner272 commented Mar 9, 2014

dpshelio commented Mar 10, 2014

aringlis commented Apr 30, 2014

Cadair commented May 12, 2014

aringlis commented May 12, 2014

Cadair commented May 12, 2014

aringlis commented May 19, 2014

ayshih commented May 19, 2014

Choose a reason for hiding this comment

ehsteve commented May 19, 2014

Cadair commented Jun 2, 2014

gunner272 commented Jun 2, 2014

Cadair commented Jun 2, 2014

coveralls commented Jun 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cadair commented Jul 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aringlis commented Jul 14, 2014

coveralls commented Jul 15, 2014

Cadair commented Aug 4, 2014

ehsteve commented Aug 25, 2014

[Review]: Clean up `parse_time` API #887

[Review]: Clean up `parse_time` API #887