-
Notifications
You must be signed in to change notification settings - Fork 23
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
Support datetime in conversion options #1139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THank you for taking care of this. PR looks good to me
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1139 +/- ##
==========================================
- Coverage 90.60% 90.60% -0.01%
==========================================
Files 129 129
Lines 8174 8193 +19
==========================================
+ Hits 7406 7423 +17
- Misses 768 770 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
class _NWBConversionOptionsEncoder(_NWBMetaDataEncoder): | ||
""" | ||
Custom JSON encoder for conversion options of the data interfaces and converters (i.e. kwargs). | ||
|
||
This encoder extends the default JSONEncoder class and provides custom serialization | ||
for certain data types commonly used in interface source data. | ||
""" | ||
|
||
def default(self, obj): | ||
|
||
# Over-write behaviors for Paths | ||
if isinstance(obj, Path): | ||
return str(obj) | ||
|
||
return super().default(obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add this to _NWBMetaDataEncoder
instead of making a new class? I'm kind of surprised this isn't the default behavior for Path objs. What is the default behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behavior is to throw an error.
Yeah, maybe we can use the same encoder for everything. Note that even before this PR we already had a _NWBSourceDataEncoder
:
neuroconv/src/neuroconv/utils/json_schema.py
Lines 46 to 60 in dcd248b
class _NWBSourceDataEncoder(_NWBMetaDataEncoder): | |
""" | |
Custom JSON encoder for data interface source data (i.e. kwargs). | |
This encoder extends the default JSONEncoder class and provides custom serialization | |
for certain data types commonly used in interface source data. | |
""" | |
def default(self, obj): | |
# Over-write behaviors for Paths | |
if isinstance(obj, Path): | |
return str(obj) | |
return super().default(obj) |
I think that the original idea was that metadata, source data and the conversion options might require different behavior so it was created that way. Check out the inheritance pattern here that kind of implements this simplification:
https://github.com/catalystneuro/neuroconv/pull/1142/files#r1856981408
Hi, @alessandratrapani .
I need to add a test but this is the reason you are seeing this error. As you might know we use json schema to do validation of the conversion options, metadata and interface arguments (source data). The json schema validation requires arguments to be string and we use encoders to accomplish this. Check out the code. I still need to add a test for this but I wanted to share this with you as quick as possible.
A temporal solution in your code is to transform the datetime object to string (see the diff here for how) and then transform it back to datetime in
add_to_nwb
. But we can merge this quickly as well once I add a test.