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

Fix Unix time table calculation to maintain file day #38

Merged
merged 4 commits into from Oct 20, 2020

Conversation

jtniehof
Copy link
Member

The Unix time table truncated start times for files to integer and used ceiling for end times. This means that a file with a timestamp late in the day (e.g. 23:59:59.600) could potentially be treated as if it had data in the next day, so for instance it would be passed in as an input for any processes that were using the next day. This PR does a truncate-to-integer in all cases, in both calculating the values for storage in the database and in converting datetimes to Unix time for lookup in the database.

Using an integer here is reasonable for speed (and not having to change database format yet again), and in general we don't operate with sub-second accuracy. We just need to do the Right Thing (or the Justifiable Thing) when there are sub-second timestamps. Because the beginning of a second is considered to be part of the second but the "end" of a second is not (i.e. second 5 includes 5.000000 but excludes 6.00000), this treatment guarantees that all moments within a given second are treated the same. (And thus, by induction, all moments within a minute, hour, day, etc.).

PR Checklist

  • Pull request has descriptive title
  • Pull request gives overview of changes
  • New code has inline comments where necessary
  • Any new modules, functions or classes have docstrings consistent with dbprocessing style
  • (N/A) Major new functionality has appropriate Sphinx documentation
  • (N/A) Added an entry to CHANGELOG if fixing a major bug or providing a major new feature
  • New features and bug fixes should have unit tests
  • (N/A) Relevant issues are linked in the description (e.g. See issue # or Closes #)

balarsen
balarsen previously approved these changes Oct 19, 2020
Copy link
Contributor

@balarsen balarsen left a comment

Choose a reason for hiding this comment

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

These look good to me, interesting issue to kind and sort

@balarsen
Copy link
Contributor

@jtniehof can you poke this?

 * This fixes the problem where day with record at 23:59:59.6 would appear
   to have data in the next day.
 * If update the conversion in one place, need to update it everywhere
   for consistency.
unit_tests/test_DButils.py Show resolved Hide resolved
@dnadeau-lanl dnadeau-lanl merged commit 20afffc into spacepy:master Oct 20, 2020
@jtniehof
Copy link
Member Author

Few things going on here.

  1. This should not have made a merge commit. Apparently git gives a "merge" button when "require up to date" is set, even if "require linear history" is set. When @balarsen hit that, the new commit then retriggered the CI
  2. Because the CI was retriggered it tripped over the problem now fixed by CircleCI: update apt cache before installing packages (fix CreateDBsabrs test) #39.

I'm going to turn off the "require up to date" and we'll just have to manually enforce "do a rebase and force push before merge" if the history looks like I think it does now...might have to do a reversion.

@balarsen
Copy link
Contributor

Few things going on here.

1. This should not have made a merge commit. Apparently git gives a "merge" button when "require up to date" is set, even if "require linear history" is set. When @balarsen hit that, the new commit then retriggered the CI

2. Because the CI was retriggered it tripped over the problem now fixed by #39.

I'm going to turn off the "require up to date" and we'll just have to manually enforce "do a rebase and force push before merge" if the history looks like I think it does now...might have to do a reversion.

OK, please do provide guidance, the button I hit was the only option. Seems like it is all good now.

@jtniehof
Copy link
Member Author

Looks like our history stayed linear, putting further discussion in #40. No reversion required, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants