-
Notifications
You must be signed in to change notification settings - Fork 35
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
Comments
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? |
@fschlueter Could you point to where you have implemented this (i.e. which file)? :-) |
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. 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 |
Something like this could be a solution:
|
We can of course add try except block and so on to make it more robust. |
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 adatetime
object is made.The root cause is that on lines 171-179 of that file, the
TinyDB
object gets initialized without theSerializationMiddleware
object that contains theDateTimeSerializer
(lines 167-168). Without it, the{TinyDate}
strings from the JSON file are not properly parsed asdatetime
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
toSerializationMiddleware
), 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.
The text was updated successfully, but these errors were encountered: