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 support for the "today" argument for UTCDateTime #2337

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ThomasLecocq
Copy link
Contributor

ThomasLecocq commented Feb 18, 2019

What does this PR do?

It adds the possibility to pass "today" to UTCDateTime in order to obtain a UTCDateTime object initialised at the beginning of the current day. The result is similar as:

UTCDateTime(UTCDateTime().date))

Why was it initiated? Any relevant Issues?

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: + DOCS
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .

@ThomasLecocq ThomasLecocq requested a review from krischer Feb 18, 2019

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 18, 2019

Hmm.. how about UTCDateTime.today() which would be inline with datetime and less magicky?

Euh.. datetime.today() actually is same as datetime.now().. xD

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor Author

ThomasLecocq commented Feb 18, 2019

because:

In [5]: datetime.datetime.today()
Out[5]: datetime.datetime(2019, 2, 18, 16, 34, 21, 233135)

is misleading, according to me...

@d-chambers

This comment has been minimized.

Copy link
Member

d-chambers commented Feb 18, 2019

Hmmmm... out of curiosity what is the use case for this?

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor Author

ThomasLecocq commented Feb 18, 2019

i.e. for

  • selecting data from a data center for everything since this morning
  • building a interator over datetime slices, starting at the beginning of the day
@d-chambers

This comment has been minimized.

Copy link
Member

d-chambers commented Feb 18, 2019

Ok makes sense. I like the idea @megies mentioned better than allowing yet another input to the standard constructor for two reasons:

1. UTCDateTime already takes a whole lot (too many) of inputs of different types and meanings resulting in an init method that is very long and complicated.

  1. One may also want the start of day of an arbitrary date, not just today. A method would make it easier to generalize this functionality.

Maybe a start_of_day method or property that returns a new UTCDateTime object at the start of the date represented by the data would be a cleaner solution? Then if you want today all you would have to do is UTCDateTime().start_of_day, or something like that.

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor Author

ThomasLecocq commented Feb 19, 2019

@d-chambers I fully agree with your comments, and indeed, the "start of the day" for any day is interesting in different cases!

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 19, 2019

@d-chambers the .start_of_day() seems like a good compromise. 👍

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor Author

ThomasLecocq commented Feb 19, 2019

OK, seem indeed more interesting to have the start_of_day, more generally useful indeed.

I also created the end_of_day to be 1ns before midnight, as it could, e.g. be useful and intuitive for

c = Client("IRIS")
c.get_waveforms("BE", "MEM", "", "HHZ", UTCDateTime().start_of_day(), UTCDateTime().end_of_day())

but this is open for discussion!

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 19, 2019

I really don't know about the end-of-day thingy. Feels a bit like introducing floating point glitches deliberately (although we're integer based for time internally) and also feels like redundancy? I like to use start of next day as end time in such situations, feels cleaner for me. If there's really a strong need in any situation it's still possible to just slap a - 1 on the time to make sure it's the earlier date..?

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor Author

ThomasLecocq commented Feb 19, 2019

@megies yep good point :)

@d-chambers

This comment has been minimized.

Copy link
Member

d-chambers commented Feb 19, 2019

Cool, looks good.

Just a thought, would it ever be useful to have a start of month, year, minute or hour? If so maybe this should be made general like so:

UTCDateTime().start_of("day")
UTCDateTime().start_of("month") 
...

where the main argument would support year, month, day, hour, minute, second.

If day is the most used, however, maybe this isn't needed. It's up to you since this is your PR 😉

I do agree that end of day stuff probably isn't needed.

@markcwill

This comment has been minimized.

Copy link
Contributor

markcwill commented Feb 20, 2019

Agree with using a method/property on the time object rather than constructor, and with @d-chambers that a generic solution might be helpful in the future with this as a specific case. Couple notes on prior art:

  1. Go time.Time has a "Truncate" method that does exactly this and it is awesome, we use it all the time for this type of "time rounding" for buckets, batching, etc: https://golang.org/pkg/time/#Time.Truncate
  2. A slightly different approach is in numpy.datetime64, which IMHO UTCDateTime should inherit/embed as the underlying time store someday? They allow casting of units to get a similar effect: https://docs.scipy.org/doc/numpy/reference/arrays.datetime.html

Looks like you have a UTCDateTime.start_of_day sugar property method that does what you want, can always implement for example UTCDateTime.truncate('day') later and call that under the hood in a refactor.

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.