search

panoptes / panoptes-utils

A set of Astronomical utility functions. PANOPTES style. :hammer_and_wrench: :telescope: :stars:
https://panoptes-utils.readthedocs.io
MIT License
4 stars 9 forks source link

Add datetime serialization #18

Closed wtgee closed 5 years ago

wtgee commented 5 years ago

LGTM. What was the motivation for switching to orjson in the first place?

Mostly for easier datetime serialization, which is now lost again (and I should make a note of). We had been using some from bson import json_util, with bson coming from mongo originally. Could also just manually handle the datetimes, like we do with astropy quantities.

Originally posted by @wtgee in https://github.com/panoptes/panoptes-utils/pull/15#issuecomment-481483624

AnthonyHorton commented 5 years ago

Ah, OK. If we jumped to Python 3.7+ it would be easy to (de)serialise manually, with datetime.isoformat() and datetime.fromisoformat()... For Python 3.6 there's no fromisoformat so would need to use datetime.strptime() instead.

wtgee commented 5 years ago

Well the issue was making it work on the arm builds for the Pi, which isn't yet at 3.7 (using Berryconda), so don't think that will work. Trying to find a good cross-platform solution with minimal work.

On Wed, Apr 10, 2019, 10:32 AM Anthony Horton <notifications@github.com wrote:

Ah, OK. If we jumped to Python 3.7+ it would be easy to (de)serialise manually, with datetime.isoformat() and datetime.fromisoformat()... For Python 3.6 there's no fromisoformat so would need to use datetime.strptime() instead.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/panoptes/panoptes-utils/issues/18#issuecomment-481489088, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEUUJD0GVv2faArCIh3V_MyJc3tT7Pjks5vfTEsgaJpZM4cl3Ry .

AnthonyHorton commented 5 years ago

Hmmm, the string formatting/parsing in the builtin datetime library is a bit of mess when I look into it. The datetime.strftime(), datetime.strptime() methods ought to make it easy to do a serialisation/deserialisation round trip, except that datetimes may or may not have a timezone and that means you're not guaranteed to be able to parse the string with the same format string that you formatted it with! Need an extra try to try two different format strings. Am working on it anyway.

wtgee commented 5 years ago

Oh, hm, I don't think this is a huge priority. Going back to bson should solve the issue pretty immediately and is the same behavior as what we currently do in POCS.

I always use dateutil.parse for date parsing because it's just easier.

AnthonyHorton commented 5 years ago

Another question, do we want the serialiser/deserialiser to work with datetime.datetime objects, astropy.time.Time objects, or both?

wtgee commented 5 years ago

Another question, do we want the serialiser/deserialiser to work with datetime.datetime objects, astropy.time.Time objects, or both?

I think it's fair for us to always convert to an astropy Time object; they are a set of astronomy utilities and we already assume quantity usage elsewhere. Serializing to json should handle both (which should be trivial).

Likewise, we always assume UTC and thus try to avoid timezone issues.

wtgee commented 5 years ago

However we do have some timezone issues I had mostly forgotten about. See https://github.com/panoptes/POCS/issues/821.

wtgee commented 5 years ago

Fixed in #7 and #5 (and elsewhere fixes).