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 tz for gt3x #200

Closed
wants to merge 2 commits into from
Closed

fix tz for gt3x #200

wants to merge 2 commits into from

Conversation

angerhang
Copy link
Member

Having spoken with the Actigrpah team, they have confirmed that all the timestamps are stored in UNIX UTC, the true UNIX time. The current expected behaviour is that:

  • For npy file, we will just read the timestamps are they are.
  • If a user provides the tz flag, we will change the epoch tz to the user input one. Otherwise we will default to Europe/London.

This PR should address: issue #198

@aidendoherty
Copy link
Member

Looks good to me but we should also ask @chanshing to review too.

I wonder if this also need to be done for GeneActiv and Axivity devices?
We can discuss this in person when we next meet in the office.

@angerhang
Copy link
Member Author

Looks good to me but we should also ask @chanshing to review too.

I wonder if this also need to be done for GeneActiv and Axivity devices? We can discuss this in person when we next meet in the office.

Thanks for the comment Aiden. All three devices deal with time zone differently. But I plan to submit a PR later this week to include a minimal test set at least for GT3X and Axivity devices.

As reported in #199, there seems to be an issue with Axivity time zone parsing as well but I have left that to Shing to fix as I am less familiar with its encoding.

@angerhang angerhang linked an issue Jun 20, 2022 that may be closed by this pull request
@angerhang
Copy link
Member Author

After some investigation, GT3X's UNIX time is in fact local time. The user will need to provide both the time shift information in hours and timezone to get the correct output. If nothing is provided, we will default everything to Europe/London.

@angerhang
Copy link
Member Author

@chanshing can you review this quickly and publish a new release when you have the time? I kinda need to get this done soon for a bunch of analyses that I have to run :D

It is probably time to set up automatic PR release for pip distribution.

@chanshing
Copy link
Member

chanshing commented Jun 21, 2022

Hang, I think we don't need offset AND timezone. Just timezone should be enough information. If the device is only storing local time, then I think it is very similar to Axivity (and will have same problem regarding DST)

@chanshing
Copy link
Member

chanshing commented Jun 21, 2022

This might be useful: https://docs.microsoft.com/en-us/dotnet/api/system.datetime.ticks?view=net-6.0
It says that .NET ticks stores ticks since Jan 0001 midnight at either UTC or some other time zone (local setting). What we need now is the inverse transform of this: function(ticks, timezone) -> time (it can be unix, UTC or any localized time)

@angerhang
Copy link
Member Author

I have spent the whole morning trying to do this. Even writing the test cases took a long time and I still haven't finished one test case yet probably due to my lack of familiarity with Java and its timezone classes. GT3X has two versions add to the complexity. Can we just merge this PR? It is better than the current version already because the current version is just sometimes wrong because of the timezone.

To get this correct with the right set of test cases, it will take 2-3 days' time. I have to prioritise other tasks at the moment.
We really should move away from merging PR without test cases. The more we build on things, the more we question if things are really going to work.

@angerhang
Copy link
Member Author

Anyhow, I will probably work with my own fork for now. Maybe revisit this at a later date.

@angerhang angerhang closed this Jun 21, 2022
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.

GT3X expected time zone behaviour
3 participants