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

Detector from dictionary does not support TinyDate #636

Open
MijnheerD opened this issue Feb 21, 2024 · 5 comments
Open

Detector from dictionary does not support TinyDate #636

MijnheerD opened this issue Feb 21, 2024 · 5 comments
Labels
help wanted Extra attention is needed invalid This doesn't seem right

Comments

@MijnheerD
Copy link
Collaborator

Describe the issue
When loading a (complete) Detector from a dictionary, by for example first dumping the JSON file into one and then loading a Detector from that dictionary, the deployment and (de)commission dates are not properly interpreted. Rather they are saved as literal strings, resulting in a TypeError when querying the stations/channels, because in lines 250-252 of "detector_base.py" a comparison between a string (coming from the detector description) and a datetime object is made.

The root cause is that on lines 171-179 of that file, the TinyDB object gets initialized without the SerializationMiddleware object that contains the DateTimeSerializer (lines 167-168). Without it, the {TinyDate} strings from the JSON file are not properly parsed as datetime objects, but are rather considered to be literal strings.

Expected behavior
I would expect the dictionary variant of the Detector to be able to handle the dates. Though this might require to change the storage option (from MemoryStorage to SerializationMiddleware), so I am not sure how easy this is.

Another option here could be to add a specific parser that changes the TinyDate strings to UNIX stamps, which can be stored in the database as integers. These can then be compared, but that would require a rewrite of the query functions to use timestamps instead of datetime objects.

At least a warning should be thrown that the dates are not properly parsed (yet) and one should use the JSON solution instead.

Additional context
This issue came up when @karenterveer implemented a complete (i.e. not a GenericDetector) description for LOFAR. Until now, we had been using a workaround when loading our GenericDetector, which consisted of first dumping the JSON file into a dictionary and then loading the Detector from that dictionary (as described in #592). In that case the time check is skipped (?) and thus we never ran into this problem.

@MijnheerD MijnheerD added bug Something isn't working help wanted Extra attention is needed invalid This doesn't seem right and removed bug Something isn't working labels Feb 21, 2024
@fschlueter
Copy link
Collaborator

Hey Mitja, in the RNO-G detector class we allow loading the detector description from a json file. Somewhere in the code we are converting the strings back to datetime objects. This might help you?

@MijnheerD
Copy link
Collaborator Author

@fschlueter Could you point to where you have implemented this (i.e. which file)? :-)

@cg-laser
Copy link
Collaborator

For more background, when reading a detector description from JSON via TinyDB, all times are converted back into time. This is because JSON can not serialize more complex objects. So this is a custom solution.
When passing the dictionary in memory, it is assumed that times are Time objects, which I think makes sense.

However, I agree that this behaviour is confusing and error prone and we should fix it. This is not time critical, so we can do a better job of finding the time objects still saved as string. Can you just add the datetime serializer also to MemoryStorage?

@fschlueter
Copy link
Collaborator

Something like this could be a solution:

import json
import datetime
import re

def _json_serial(obj):
    """JSON serializer for objects not serializable by default json code"""

    if isinstance(obj, (datetime.datetime, datetime.date)):
        return obj.isoformat()
    else:
        raise TypeError ("Type %s not serializable" % type(obj))

def _json_deserial(obj):
    """JSON serializer for objects not serializable by default json code"""
    for key in obj:
        if re.search("time", key) is not None:
            obj[key] = datetime.datetime.fromisoformat(obj[key])
    return obj


dic = {"a": {"time": datetime.datetime.now()}}

print(dic)

dic2 = json.loads(json.dumps(dic, default=_json_serial), object_hook=_json_deserial)

print(dic2)

@fschlueter
Copy link
Collaborator

We can of course add try except block and so on to make it more robust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants