-
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
restore public status of NWBMetaDataEncoder
encoder
#1142
Changes from 17 commits
eb847c8
13c5024
862f42d
7d80bc7
e24ad38
f1c5e19
b901c33
7b9294e
7efa366
7340644
8e8a1a3
aecf92a
15b952e
eba9c45
f56bb7d
242f667
3b741e0
6577404
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,13 +16,8 @@ | |
from pynwb.icephys import IntracellularElectrode | ||
|
||
|
||
class _NWBMetaDataEncoder(json.JSONEncoder): | ||
""" | ||
Custom JSON encoder for NWB metadata. | ||
|
||
This encoder extends the default JSONEncoder class and provides custom serialization | ||
for certain data types commonly used in NWB metadata. | ||
""" | ||
class _GenericNeuroconvEncoder(json.JSONEncoder): | ||
"""Generic JSON encoder for NeuroConv data.""" | ||
|
||
def default(self, obj): | ||
""" | ||
|
@@ -36,45 +31,38 @@ def default(self, obj): | |
if isinstance(obj, np.generic): | ||
return obj.item() | ||
|
||
# Numpy arrays should be converted to lists | ||
if isinstance(obj, np.ndarray): | ||
return obj.tolist() | ||
|
||
# Over-write behaviors for Paths | ||
if isinstance(obj, Path): | ||
return str(obj) | ||
|
||
# The base-class handles it | ||
return super().default(obj) | ||
|
||
|
||
class _NWBSourceDataEncoder(_NWBMetaDataEncoder): | ||
class _NWBMetaDataEncoder(_GenericNeuroconvEncoder): | ||
""" | ||
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. | ||
Custom JSON encoder for NWB metadata. | ||
""" | ||
|
||
def default(self, obj): | ||
|
||
# Over-write behaviors for Paths | ||
if isinstance(obj, Path): | ||
return str(obj) | ||
|
||
return super().default(obj) | ||
class _NWBSourceDataEncoder(_GenericNeuroconvEncoder): | ||
""" | ||
Custom JSON encoder for data interface source data (i.e. kwargs). | ||
""" | ||
|
||
|
||
class _NWBConversionOptionsEncoder(_NWBMetaDataEncoder): | ||
class _NWBConversionOptionsEncoder(_GenericNeuroconvEncoder): | ||
""" | ||
Comment on lines
-46
to
59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a strange pattern. Why define child classes that are identical to the parent class? Why not just use the parent class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They might diverge and we might not want the same behavior for each of those cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that they are private though, I don't mind having the simple case as you want it and then refactoring if and when we need it. |
||
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) | ||
# This is used in the Guide so we will keep it public. | ||
NWBMetaDataEncoder = _NWBMetaDataEncoder | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a strange pattern. Why not just define it as public to begin with? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to keep things private by default in general. Then we don't have to worry when we refactor, move stuff, change signature, etc. It is less work for us for something that is not our core service and it frees resources for more important stuff. The problem here is that Ryan was using that one, then we made it private at some point in June which broke something on its end. We are making it public for him here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the issue here: Magic was used to solve the compatibility break from path and strings to pydantic with aliases and then that magic broke something else. This is restoring / addressing this. |
||
|
||
|
||
def get_base_schema( | ||
|
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.
@bendichter here.