Turbo87 / aerofiles

waypoint, task, tracklog readers and writers for aviation
http://aerofiles.readthedocs.org/
MIT License
45 stars 27 forks source link

timezone aware fixes? #299

Closed GliderGeek closed 2 months ago

GliderGeek commented 3 months ago

would it be an idea to add the possibility of returning timezone aware datetime.datetime objects instead of datetime.time objects in the fix records?

in IGC files the time is recorded in UTC, leading to potentially weird situations when your are in a timezone far away from UTC. You can have fixes going over the date line, from 23:59 -> 00:00

this recently lead to a problem in my program, trying to analyze glider flights for the gliding world championship in uvalde (UTC-5).

Adding this optional conversion would necessitate less conversions in downstream packages/programs. I see from the reader that it finds utc_date from one of the H records. ~Im not sure if this field is required in the format.~ this source says this record is required and always declared before the B records, so this would be possible without having to loop in a weird order.

Curious to hear how you look at this 😄

bubeck commented 3 months ago

This is also great idea! Please send PR, to merge. Thanks!

GliderGeek commented 3 months ago

Nice to hear you like it! What would be an appropriate implementation? An optional flag on the constructor, similar to skip_duplicates? What would be an appropriate name: tz_aware_fixes?

bubeck commented 3 months ago

Do we really need a flag or should this be standard behaviour? I don't see a case where the old Implementation should be preferred. What do you think?

GliderGeek commented 3 months ago

Also fine, but it will break backwards compatibility so i suppose an increase in major version number would be justified

bubeck commented 3 months ago

Agree

sylvainvdm commented 3 months ago

@GliderGeek @bubeck please see #302 for the requested fixes according to the implementation described above

bubeck commented 3 months ago

I am on holiday until 2024-09-09. Please give me some time after this date to look at.

bubeck commented 3 months ago

https://github.com/Turbo87/aerofiles/blob/184eb240654df2fc38e4bfd80332b6ccb0c67685/aerofiles/igc/reader.py#L204

I haven't looked at your implementation yet. However here are my general thoughts:

What do you think?

sylvainvdm commented 3 months ago
  • Add TZ=UTC to B record and also HFDTE (and more?) to change them from naive to aware.
  • Keep 'time' in B as datetime.time to be compatible, only make aware to UTC
  • Add 'localtime' as datetime.datetime with TZ from header (or UTC if HFTZN is not available) and do all the magic here with time overrun.
  1. Agreed and done
  2. I agree with your statement in https://github.com/Turbo87/aerofiles/issues/299#issuecomment-2322870854 , where you indicate that this behaviour is always wished. With a major version upgrade this should be covered. Otherwise an extra column is needed
  3. Can be an option. You are essentially storing the same information twice in the dataset, might be a drawback

I'm aligned with your initial thoughts, seems logical to me!

bubeck commented 2 months ago

Done with https://github.com/Turbo87/aerofiles/pull/307