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

PPSD: change internal representation of timestamps to integer nanosecond #2045

Merged
merged 3 commits into from Jan 24, 2018

Conversation

Projects
None yet
3 participants
@megies
Member

megies commented Jan 12, 2018

This changes all internally used timestamps in PPSD to POSIX timestamps as integer nanoseconds, eliminating all potential floating point inaccuracies when e.g. comparing time stamps etc.

The only drawback is that npz files written on newer obspy versions will result in exceptions when reading on older obspy versions (the other way around should work fine). But I think the positive effect outweighs this drawback.

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"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests. no new features
  • Any new or changed features have are fully documented. no new features
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

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

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

@megies megies requested a review from ThomasLecocq Jan 12, 2018

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 12, 2018

Member

Also see #2034 for examples of unexpected behavior associated with floating point issues.

Member

megies commented Jan 12, 2018

Also see #2034 for examples of unexpected behavior associated with floating point issues.

@krischer

Looks pretty good to me.

How about adding some future proofing by checking if the ppsd_version of the .npz files is greater than the current one and raise a warning that reading might go wrong?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 16, 2018

Member

Good idea.. actually, I'm tempted to even raise an Exception since it's really rather likely that something will go wrong in that case..

Member

megies commented Jan 16, 2018

Good idea.. actually, I'm tempted to even raise an Exception since it's really rather likely that something will go wrong in that case..

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 16, 2018

Member

@krischer, I've added raising an exception in this case and will also do a PR for maintenance branch that at least shows a warning, so that the 1.1.x line is also covered with this..

Member

megies commented Jan 16, 2018

@krischer, I've added raising an exception in this case and will also do a PR for maintenance branch that at least shows a warning, so that the 1.1.x line is also covered with this..

Show outdated Hide outdated obspy/signal/spectral_estimation.py Outdated
Show outdated Hide outdated CHANGELOG.txt Outdated
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 16, 2018

Member

Made those wording changes and force-pushed.

Member

megies commented Jan 16, 2018

Made those wording changes and force-pushed.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 18, 2018

Member

Rebased and removed the exception for newer npz, was delegated to #2051. Hope I didn't screw up the merge conflict resolution, always a bit tricky when indent levels change (due to the context manager used now)

hash before rebasing: 7d684ba

Member

megies commented Jan 18, 2018

Rebased and removed the exception for newer npz, was delegated to #2051. Hope I didn't screw up the merge conflict resolution, always a bit tricky when indent levels change (due to the context manager used now)

hash before rebasing: 7d684ba

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 18, 2018

Member

Some tests fail now.

Member

krischer commented Jan 18, 2018

Some tests fail now.

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Jan 19, 2018

Contributor

Some tests fail now.

One of these is the test included in the PPSD fix we merged some days ago.

Contributor

Jollyfant commented Jan 19, 2018

Some tests fail now.

One of these is the test included in the PPSD fix we merged some days ago.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 19, 2018

Member

One of these is the test included in the PPSD fix we merged some days ago.

Ah OK.. I rebased but didn't check that new test.. and somehow must've messed up the conflict resolution during rebasing, that npz version check somehow got lost.. -_-

Member

megies commented Jan 19, 2018

One of these is the test included in the PPSD fix we merged some days ago.

Ah OK.. I rebased but didn't check that new test.. and somehow must've messed up the conflict resolution during rebasing, that npz version check somehow got lost.. -_-

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Jan 19, 2018

Contributor

It could be because of the change in ._times_processed to nanoseconds representation.

Contributor

Jollyfant commented Jan 19, 2018

It could be because of the change in ._times_processed to nanoseconds representation.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 19, 2018

Member

Yeah, sure. All issues should be fixed now, hopefully.

Member

megies commented Jan 19, 2018

Yeah, sure. All issues should be fixed now, hopefully.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 19, 2018

Member

OK, looks like this one's ready now. I'm a bit anxious about the internal time representation change.. but I think it's better in the long term and hopefully I thought about all issues that might arise when reading/adding older npz files..

Member

megies commented Jan 19, 2018

OK, looks like this one's ready now. I'm a bit anxious about the internal time representation change.. but I think it's better in the long term and hopefully I thought about all issues that might arise when reading/adding older npz files..

megies added some commits Jan 12, 2018

PPSD: change internal representation of timestamps to integer nanosecond
POSIX timestamps

also changes how data is serialized to npz and accomodates loading old
npz with float seconds POSIX timestamps
@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 23, 2018

Member

I fixed a conflict in the changelog and rebased. Can be merged from my point of view if CI is green.

Member

krischer commented Jan 23, 2018

I fixed a conflict in the changelog and rebased. Can be merged from my point of view if CI is green.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 24, 2018

Member

Alright, let's do it.. 🙈

Member

megies commented Jan 24, 2018

Alright, let's do it.. 🙈

@megies megies merged commit 4cda8a3 into master Jan 24, 2018

2 of 4 checks passed

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 ppsd_times_nanoseconds branch Jan 24, 2018

@megies megies referenced this pull request Feb 13, 2018

Open

The future of time #2072

@megies megies referenced this pull request Apr 10, 2018

Open

Maintenance needed on "obspy-scan" #2102

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment