-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
Added a subclass of astropy.time to support utime #2409
Conversation
Will need a test too.
…On Wed, Jan 17, 2018 at 11:34 AM, James Mason ***@***.***> wrote:
Implemented handling of utime by creating a custom subclass of
astropy.time.Time, which uses the third code example from their
documentation
<http://docs.astropy.org/en/stable/time/#writing-a-custom-format> as a
template.
------------------------------
You can view, comment on, or merge this pull request online at:
#2409
Commit Summary
- Added a subclass of astropy.time to support utime
File Changes
- *A* sunpy/time/utime.py
<https://github.com/sunpy/sunpy/pull/2409/files#diff-0> (21)
Patch Links:
- https://github.com/sunpy/sunpy/pull/2409.patch
- https://github.com/sunpy/sunpy/pull/2409.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2409>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA8CF8DfcOADbB6UmYlE6RNz1aAuH7zOks5tLiEcgaJpZM4Rhn_i>
.
|
Also, maybe a short explanation of why this is needed.
On Wed, Jan 17, 2018 at 11:40 AM, Jack Ireland <jackireland@gmail.com>
wrote:
… Will need a test too.
On Wed, Jan 17, 2018 at 11:34 AM, James Mason ***@***.***>
wrote:
> Implemented handling of utime by creating a custom subclass of
> astropy.time.Time, which uses the third code example from their
> documentation
> <http://docs.astropy.org/en/stable/time/#writing-a-custom-format> as a
> template.
> ------------------------------
> You can view, comment on, or merge this pull request online at:
>
> #2409
> Commit Summary
>
> - Added a subclass of astropy.time to support utime
>
> File Changes
>
> - *A* sunpy/time/utime.py
> <https://github.com/sunpy/sunpy/pull/2409/files#diff-0> (21)
>
> Patch Links:
>
> - https://github.com/sunpy/sunpy/pull/2409.patch
> - https://github.com/sunpy/sunpy/pull/2409.diff
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#2409>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA8CF8DfcOADbB6UmYlE6RNz1aAuH7zOks5tLiEcgaJpZM4Rhn_i>
> .
>
|
Will do. It's needed because @Cadair told me so? 😛 |
Knowing very little about this, can you construct it with the float? A test for that use would be great as well. (Seeing how that's the most likely use of this, not to save out utime as we are not monsters :p ) |
I was afraid of you asking for that.. that's what I spent a couple hours on yesterday (but for a new yyyydoy_sod subclass) and that is much harder to figure out since I haven't found any comprehensive astropy examples. I'll keep at it though. |
This PR is now dependent on the resolution of issue 7092 in astropy that will enable, in this case, utime as an input. |
sunpy/time/utime.py
Outdated
|
||
|
||
class TimeUTime(TimeFromEpoch): | ||
"""Seconds from 1979-01-01 00:00:00 UTC. Same as Unix time |
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.
I forget, should there not be a new line after the docstring quotes?
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.
PEP257 doesn't specify but I can change it if that's a standard recommended in sunpy or in the science stack.
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.
It's not an issue then!
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.
It's a Cadair standard, which means most of SunPy is like that ;)
sunpy/time/utime.py
Outdated
from astropy.time import Time | ||
t = Time('2000-01-01T13:53:23') | ||
print(t.utime) | ||
>>> 662738003.0 |
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.
This looks wrong for example syntax.
I think it should be somrthing like:
Examples
--------
>>> code line 1
>>> code line 2
I forget how you add the output.
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.
I think you're right. I couldn't find a PEP standard but I found a random example of how it's done in scipy. For example:
>>> from scipy.interpolate import BSpline
>>> k = 2
>>> t = [0, 1, 2, 3, 4, 5, 6]
>>> c = [-1, 2, 0, -1]
>>> spl = BSpline(t, c, k)
>>> spl(2.5)
array(1.375)
>>> bspline(2.5, t, c, k)
1.375
sunpy/time/utime.py
Outdated
This time format is included for historical reasons. Some | ||
people in solar physics prefer using this epoch. | ||
|
||
Example: |
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.
I think example should be underlined
Example
-------
|
||
|
||
def test_utime_random_date(): | ||
assert Time('2018-01-13T13:32:56').utime == 1231853576.0 |
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.
Could you please add a test which is marked with @pytest.mark.xfail
of the reverse conversion? i.e. the open astropy issue?
sunpy/time/tests/test_utime.py
Outdated
from __future__ import absolute_import, division, print_function | ||
|
||
from astropy.time import Time | ||
|
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.
should this not explicitly import the utime format to register it with astropy?
@jmason86 can you merge master into this to fix the CI please? |
Can you add a changelog entry for this please? Other than that this looks great. |
[ci skip]
Implemented handling of utime by creating a custom subclass of
astropy.time.Time
, which uses the third code example from their documentation as a template.