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

UTCDateTime: add public property to access nanosecond integer POSIX timestamp #2046

Merged
merged 3 commits into from Jan 18, 2018

Conversation

Projects
None yet
4 participants
@megies
Member

megies commented Jan 12, 2018

Since this is now the internal representation of UTCDateTime, I think it makes sense for user code to use that to serialize UTCDateTime objects, and we should therefore offer a public property to access this. Also implementing __int__ for this would be handy, but I fear that might be way too confusing alongside the existing __float__ which returns the timestamp in seconds..

Why was it initiated? Any relevant Issues?

e.g. #2045, #2036

PR Checklist

  • Correct base branch selected? master for new fetures, 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 .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • maybe should add doc to new property

@megies megies added the .core label Jan 12, 2018

@megies megies added this to the 1.2.0 milestone Jan 12, 2018

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Jan 13, 2018

Member

While you are working with the nanoseconds, might I also suggest rounding the ns to the specified precision upon init'ing the UTCDateTime object? If the precision is specified, anything below that is just noise that may cause surprising behavior. It also would mean everything required to fully represent the data contained in the object will be given by the string representation. This would help alleviate some of the surprising behaviors in described in #2034.

You should be able to change these lines:

def _set_ns(self, value):
if not isinstance(value, int):
raise TypeError('nanoseconds must be set as int/long type')
self.__ns = value

to this:

 def _set_ns(self, value): 
     if not isinstance(value, int): 
         raise TypeError('nanoseconds must be set as int/long type') 
     self.__ns = round(value, self.precision - 9) 

Also, why not replace all instances of _ns with ns ? It would be a bit redundant to have a public and a private method that do exactly the same thing.

Member

d-chambers commented Jan 13, 2018

While you are working with the nanoseconds, might I also suggest rounding the ns to the specified precision upon init'ing the UTCDateTime object? If the precision is specified, anything below that is just noise that may cause surprising behavior. It also would mean everything required to fully represent the data contained in the object will be given by the string representation. This would help alleviate some of the surprising behaviors in described in #2034.

You should be able to change these lines:

def _set_ns(self, value):
if not isinstance(value, int):
raise TypeError('nanoseconds must be set as int/long type')
self.__ns = value

to this:

 def _set_ns(self, value): 
     if not isinstance(value, int): 
         raise TypeError('nanoseconds must be set as int/long type') 
     self.__ns = round(value, self.precision - 9) 

Also, why not replace all instances of _ns with ns ? It would be a bit redundant to have a public and a private method that do exactly the same thing.

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Jan 13, 2018

Member

round will return a float under Python 2.7:

Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:53:40) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> aa=34234234234324
>>> round(aa, -3)
34234234234000.0

I agree with changing _ns to ns.

Member

barsch commented Jan 13, 2018

round will return a float under Python 2.7:

Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:53:40) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> aa=34234234234324
>>> round(aa, -3)
34234234234000.0

I agree with changing _ns to ns.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 13, 2018

Member

While you are working with the nanoseconds, might I also suggest rounding the ns to the specified precision upon init'ing the UTCDateTime object?

That would drastically change the current logic, which is to always retain full info on the timestamp and only respect precision during comparison operations. This is definitely something that would have to be discussed in detail in a dedicated issue (and I expect mixed opinions).

Also, why not replace all instances of _ns with ns ? It would be a bit redundant to have a public and a private method that do exactly the same thing.

Yeah, but I know that code is already around that uses ._ns (I'm using it so far, for lack of the public method), so this would need a deprecation cycle, and to be honest, I'm not sure it's worth it to remove it.

Member

megies commented Jan 13, 2018

While you are working with the nanoseconds, might I also suggest rounding the ns to the specified precision upon init'ing the UTCDateTime object?

That would drastically change the current logic, which is to always retain full info on the timestamp and only respect precision during comparison operations. This is definitely something that would have to be discussed in detail in a dedicated issue (and I expect mixed opinions).

Also, why not replace all instances of _ns with ns ? It would be a bit redundant to have a public and a private method that do exactly the same thing.

Yeah, but I know that code is already around that uses ._ns (I'm using it so far, for lack of the public method), so this would need a deprecation cycle, and to be honest, I'm not sure it's worth it to remove it.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Jan 13, 2018

Member

That would drastically change the current logic, which is to always retain full info on the timestamp and only respect precision during comparison operations.

Point taken, I will open a dedicated issue when I get a few minutes so discussion can take place.

Member

d-chambers commented Jan 13, 2018

That would drastically change the current logic, which is to always retain full info on the timestamp and only respect precision during comparison operations.

Point taken, I will open a dedicated issue when I get a few minutes so discussion can take place.

@krischer

I'm also for this change and without deprecating _ns - too much hassle and IMHO not worth it.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Jan 15, 2018

Member

Hey @barsch, using the round function with a negative number works in python 2.7 (just returns a float as you observed), but doesn't work with obspy because the future.builtins wild card import replaces the built-in round with new_round, which has the following few lines:
https://github.com/PythonCharmers/python-future/blob/39a066ed8c29a0b3a32adac2ffd407119fe9ea6d/src/future/builtins/newround.py#L32-L33.

Member

d-chambers commented Jan 15, 2018

Hey @barsch, using the round function with a negative number works in python 2.7 (just returns a float as you observed), but doesn't work with obspy because the future.builtins wild card import replaces the built-in round with new_round, which has the following few lines:
https://github.com/PythonCharmers/python-future/blob/39a066ed8c29a0b3a32adac2ffd407119fe9ea6d/src/future/builtins/newround.py#L32-L33.

@barsch

barsch approved these changes Jan 16, 2018

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 16, 2018

Member

@d-chambers if you want to put work into this (future related stuff), it's probably better spent in #1613.. (hoping that it's not too ridiculously outdated already.. :-/)

Member

megies commented Jan 16, 2018

@d-chambers if you want to put work into this (future related stuff), it's probably better spent in #1613.. (hoping that it's not too ridiculously outdated already.. :-/)

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 17, 2018

Member

Rebased on current master

Member

megies commented Jan 17, 2018

Rebased on current master

@krischer

Could you maybe add a super tiny test? Otherwise feel free to merge it!

UTCDateTime: add public property to access nanosecond integer POSIX
timestamp which is also the internal representation of UTCDateTime
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 17, 2018

Member

Added test + rebased. Good to merge on nod from CI.

Member

megies commented Jan 17, 2018

Added test + rebased. Good to merge on nod from CI.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 18, 2018

Member

Curious if properties get documented by our Sphinx.. so +DOCS

edit: yes they get documented :-)

Member

megies commented Jan 18, 2018

Curious if properties get documented by our Sphinx.. so +DOCS

edit: yes they get documented :-)

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 18, 2018

Member

CI looks good, merging

Member

megies commented Jan 18, 2018

CI looks good, merging

@megies megies merged commit 32feb03 into master Jan 18, 2018

2 of 5 checks passed

docs-buildbot Build succeeded, but there are warnings/errors:
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@megies megies deleted the utcdatetime_ns_public branch Jan 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment