spacepy / dbprocessing

Automated processing controller for heliophysics data
5 stars 4 forks source link

Fix Unix time table calculation to maintain file day #38

Closed jtniehof closed 3 years ago

jtniehof commented 3 years ago

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

balarsen commented 3 years ago

@jtniehof can you poke this?

jtniehof commented 3 years ago

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.

balarsen commented 3 years ago

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 commented 3 years ago

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