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

add SEP 9 for astropy Time support #32

Merged
merged 4 commits into from
Sep 5, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
81 changes: 81 additions & 0 deletions SEP-0009.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# SEP-0009 - Astropy Time

| SEP | 9 |
|---------------|-------------------------------------------|
| title | Astropy Time |
| author(s) | Stuart Mumford |
| contact email | stuart@cadair.com |
| date-creation | 2018-05-30 |
| type | standard |
| status | discussion |
| discussion | https://github.com/sunpy/sunpy/issues/993 |

# Introduction

This SEP proposes that SunPy transitions all internal, and external use of a
Python object to represent a date or time from the standard library `datetime`
module to the classes provided in `astropy.time`. The main classes being
replacing `datetime.date` and `datetime.datetime` with `astropy.Time` and
replacing `datetime.timedelta` with `astropy.time.TimeDelta`.

# Detailed Description

Since its inception the SunPy core library has used `datetime` for handling
Copy link
Member

Choose a reason for hiding this comment

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

make clear that all of these features ARE provided by astropy.time

Copy link
Member

Choose a reason for hiding this comment

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

I don't see this comment addressed yet

Copy link
Member Author

Choose a reason for hiding this comment

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

Now added.

time. This is a very useful standard library module, however it lacks some key
benefits that are provided by Astropy and are useful to the solar community:

- Support for non-UTC time scales. This is especially useful for JSOC and HMI data which use the TAI time scale rather than UTC.
- Support for high (double-double) precision time representation and arithmetic.
- Support for leap seconds.
- Support for different representations of time, i.e. Julian Day, unix time, GPS.
- Support for reading and writing the new FITS Time standard.
- Support for light travel time calculations for different locations on Earth.
- Support for custom Time scales and formats, i.e. utime.
Copy link
Member

Choose a reason for hiding this comment

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

could you add a mention that astropy.time can be converted to datetime.

Copy link
Member

Choose a reason for hiding this comment

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

agree with @ehsteve about this point


The `astropy.time.Time` object can be converted to many different
representations of time, i.e. different pre-defined or custom string formats,
`datetime.datetime` objects. This allows users to convert back to `datetime`
objects if needed.


## Accepting Time as Input

All over SunPy when time-like objects are accepted as input, this input must be
sanitised with the `sunpy.time.parse_time` function. This function currently
returns a `datetime.datetime` object. The major API change proposed by the
transition to `astropy.time` is the return value of the `parse_time` function
will become `astropy.time.Time`. It is also proposed that the function signature
of `parse_time` be standardised with the signature of `astropy.time.Time` for
Copy link
Member

Choose a reason for hiding this comment

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

should state to always use astropy time objects inside of code unless there is a good reason not to.

clarity, the details of this are left to the implementation.

The `astropy.time.Time` object should be used as an internal representation of
time where ever possible, to ensure consistency of things like leap seconds
which would not work if `datetime` were used internally.


## Other Changes

Many other parts of SunPy will need to be adapted to work with `astropy.time`
and convert to datetime in as few places as possible to maintain the advantages
of the Astropy representation. This will result in the return values of any
functions returning time objects also becoming `astropy.time.Time` objects. It
is not expected that there will be any significant changes in functionality.


## sunpy.timeseries
Copy link
Member

Choose a reason for hiding this comment

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

Call this an exception but state that it will fixed in the future when we use astropy tables.


It should be noted that because `sunpy.timeseries.GenericTimeseries` is using
Pandas it will not be changed to use `astropy.time` it would be necessary to
migrate from Pandas to `astropy.table` to gain the advantages of `astropy.time`.
Timeseries data are currently not sufficiently supported by `astropy.table`.
Timeseries support in Astropy table is proposed to be added in
[APE 9](https://github.com/astropy/astropy-APEs/pull/12). It is expected that any
future implementation of timeseries in Astropy will be evaluated and timeseries
potentially adapted to use it. This would be the subject of a separate SEP.

The API of the `sunpy.timeseries` module will be converted to use `astropy.time`
as appropriate although the actual temporal index of the data (which is the
pandas dataframe) will not be changed.

# Decision Rationale
This is a great idea because it's already being implemented. ;)
Copy link
Member

Choose a reason for hiding this comment

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

Something about why now. "SunPy 1.0 is intended to stabilize the API. This SEP will introduce a stable implementation of time that already works with Astropy code. SunPy relies on Astropy code in many places, and this change will therefore make SunPy code maintenance simpler."

Copy link
Member Author

Choose a reason for hiding this comment

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

This section is to be written after acceptance. If anyone has comments on this wording we can adapt it and add it after approval.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, my comment is not part of the decision to adopt, but rather a reason to adopt.