nu-radio / NuRadioMC

A Monte Carlo simulation package for radio neutrino detectors
GNU General Public License v3.0
33 stars 35 forks source link

Detector from dictionary does not support TinyDate #636

Open MijnheerD opened 7 months ago

MijnheerD commented 7 months ago

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.

fschlueter commented 7 months ago

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 commented 7 months ago

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

cg-laser commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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