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

restore public status of NWBMetaDataEncoder encoder #1142

Merged
merged 18 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

## Bug Fixes
* datetime objects now can be validated as conversion options [#1139](https://github.com/catalystneuro/neuroconv/pull/1126)
* Make `NWBMetaDataEncoder` public again [PR #1142](https://github.com/catalystneuro/neuroconv/pull/1142)
* Fix a bug where data in `DeepLabCutInterface` failed to write when `ndx-pose` was not imported. [#1144](https://github.com/catalystneuro/neuroconv/pull/1144)
* `SpikeGLXConverterPipe` converter now accepts multi-probe structures with multi-trigger and does not assume a specific folder structure [#1150](https://github.com/catalystneuro/neuroconv/pull/1150)

Expand Down
2 changes: 2 additions & 0 deletions docs/api/utils.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ Dictionaries
JSON Schema
-----------
.. automodule:: neuroconv.utils.json_schema
:members:
:exclude-members: NWBMetaDataEncoder

Common Reused Types
-------------------
Expand Down
3 changes: 1 addition & 2 deletions src/neuroconv/basedatainterface.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,11 @@
)
from .tools.nwb_helpers._metadata_and_file_helpers import _resolve_backend
from .utils import (
_NWBMetaDataEncoder,
get_json_schema_from_method_signature,
load_dict_from_file,
)
from .utils.dict import DeepDict
from .utils.json_schema import _NWBSourceDataEncoder
from .utils.json_schema import _NWBMetaDataEncoder, _NWBSourceDataEncoder


class BaseDataInterface(ABC):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@

from .nvt_utils import read_data, read_header
from ....basetemporalalignmentinterface import BaseTemporalAlignmentInterface
from ....utils import DeepDict, _NWBMetaDataEncoder, get_base_schema
from ....utils import DeepDict, get_base_schema
from ....utils.json_schema import _NWBMetaDataEncoder
from ....utils.path import infer_path


Expand Down
5 changes: 2 additions & 3 deletions src/neuroconv/tools/nwb_helpers/_metadata_and_file_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from datetime import datetime
from pathlib import Path
from typing import Literal, Optional
from warnings import warn

from hdmf_zarr import NWBZarrIO
from pydantic import FilePath
Expand All @@ -26,7 +25,7 @@ def get_module(nwbfile: NWBFile, name: str, description: str = None):
"""Check if processing module exists. If not, create it. Then return module."""
if name in nwbfile.processing:
if description is not None and nwbfile.processing[name].description != description:
warn(
warnings.warn(
"Custom description given to get_module does not match existing module description! "
"Ignoring custom description."
)
Expand Down Expand Up @@ -157,7 +156,7 @@ def _attempt_cleanup_of_existing_nwbfile(nwbfile_path: Path) -> None:
# Windows in particular can encounter errors at this step
except PermissionError: # pragma: no cover
message = f"Unable to remove NWB file located at {nwbfile_path.absolute()}! Please remove it manually."
warn(message=message, stacklevel=2)
warnings.warn(message=message, stacklevel=2)


@contextmanager
Expand Down
2 changes: 1 addition & 1 deletion src/neuroconv/tools/testing/data_interface_mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
configure_backend,
get_default_backend_configuration,
)
from neuroconv.utils import _NWBMetaDataEncoder
from neuroconv.utils.json_schema import _NWBMetaDataEncoder


class DataInterfaceTestMixin:
Expand Down
2 changes: 1 addition & 1 deletion src/neuroconv/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
load_dict_from_file,
)
from .json_schema import (
_NWBMetaDataEncoder,
NWBMetaDataEncoder,
fill_defaults,
get_base_schema,
get_metadata_schema_for_icephys,
Expand Down
44 changes: 16 additions & 28 deletions src/neuroconv/utils/json_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bendichter here.

"""Generic JSON encoder for NeuroConv data."""

def default(self, obj):
"""
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

@h-mayorquin h-mayorquin Dec 9, 2024

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the issue here:
#1141

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(
Expand Down
2 changes: 1 addition & 1 deletion tests/test_minimal/test_tools/test_expand_paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from neuroconv.tools import LocalPathExpander
from neuroconv.tools.path_expansion import construct_path_template
from neuroconv.tools.testing import generate_path_expander_demo_ibl
from neuroconv.utils import _NWBMetaDataEncoder
from neuroconv.utils.json_schema import _NWBMetaDataEncoder


def create_test_directories_and_files(
Expand Down
2 changes: 1 addition & 1 deletion tests/test_minimal/test_utils/test_json_schema_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@
from pynwb.ophys import ImagingPlane, TwoPhotonSeries

from neuroconv.utils import (
_NWBMetaDataEncoder,
dict_deep_update,
fill_defaults,
get_schema_from_hdmf_class,
load_dict_from_file,
)
from neuroconv.utils.json_schema import _NWBMetaDataEncoder


def compare_dicts(a: dict, b: dict):
Expand Down
Loading