From f207b42d017b0687f23df65020a53e8afc609182 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Mon, 16 Dec 2024 14:46:45 -0600 Subject: [PATCH 01/16] basic work --- src/neuroconv/basedatainterface.py | 48 ++++++++++++++++++++---------- src/neuroconv/nwbconverter.py | 42 ++++++++++++++++++-------- 2 files changed, 62 insertions(+), 28 deletions(-) diff --git a/src/neuroconv/basedatainterface.py b/src/neuroconv/basedatainterface.py index d9e9dc11e..cd57a9d6f 100644 --- a/src/neuroconv/basedatainterface.py +++ b/src/neuroconv/basedatainterface.py @@ -17,7 +17,10 @@ make_nwbfile_from_metadata, make_or_load_nwbfile, ) -from .tools.nwb_helpers._metadata_and_file_helpers import _resolve_backend +from .tools.nwb_helpers._metadata_and_file_helpers import ( + _resolve_backend, + configure_and_write_nwbfile, +) from .utils import ( get_json_schema_from_method_signature, load_dict_from_file, @@ -195,21 +198,36 @@ def run_conversion( self.validate_metadata(metadata=metadata, append_mode=append_mode) - with make_or_load_nwbfile( - nwbfile_path=nwbfile_path, - nwbfile=nwbfile, - metadata=metadata, - overwrite=overwrite, - backend=backend, - verbose=getattr(self, "verbose", False), - ) as nwbfile_out: - if no_nwbfile_provided: - self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, **conversion_options) - - if backend_configuration is None: - backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend) + if not append_mode: - configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration) + if no_nwbfile_provided: + nwbfile = self.create_nwbfile(metadata=metadata, **conversion_options) + else: + self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, **conversion_options) + + configure_and_write_nwbfile( + nwbfile=nwbfile, + output_filepath=nwbfile_path, + backend=backend, + backend_configuration=backend_configuration, + ) + + else: # We are only using the context in append mode, see #1143 + with make_or_load_nwbfile( + nwbfile_path=nwbfile_path, + nwbfile=nwbfile, + metadata=metadata, + overwrite=overwrite, + backend=backend, + verbose=getattr(self, "verbose", False), + ) as nwbfile_out: + if no_nwbfile_provided: + self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, **conversion_options) + + if backend_configuration is None: + backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend) + + configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration) @staticmethod def get_default_backend_configuration( diff --git a/src/neuroconv/nwbconverter.py b/src/neuroconv/nwbconverter.py index 2d70cf8ee..62604eea3 100644 --- a/src/neuroconv/nwbconverter.py +++ b/src/neuroconv/nwbconverter.py @@ -14,6 +14,7 @@ from .tools.nwb_helpers import ( HDF5BackendConfiguration, ZarrBackendConfiguration, + configure_and_write_nwbfile, configure_backend, get_default_backend_configuration, get_default_nwbfile_metadata, @@ -261,21 +262,36 @@ def run_conversion( self.temporally_align_data_interfaces() - with make_or_load_nwbfile( - nwbfile_path=nwbfile_path, - nwbfile=nwbfile, - metadata=metadata, - overwrite=overwrite, - backend=backend, - verbose=getattr(self, "verbose", False), - ) as nwbfile_out: - if no_nwbfile_provided: - self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, conversion_options=conversion_options) + if not append_mode: - if backend_configuration is None: - backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend) + if no_nwbfile_provided: + nwbfile = self.create_nwbfile(metadata=metadata, conversion_options=conversion_options) + else: + self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, conversion_options=conversion_options) + + configure_and_write_nwbfile( + nwbfile=nwbfile, + output_filepath=nwbfile_path, + backend=backend, + backend_configuration=backend_configuration, + ) - configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration) + else: # We are only using the context in append mode, see #1143 + with make_or_load_nwbfile( + nwbfile_path=nwbfile_path, + nwbfile=nwbfile, + metadata=metadata, + overwrite=overwrite, + backend=backend, + verbose=getattr(self, "verbose", False), + ) as nwbfile_out: + if no_nwbfile_provided: + self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, conversion_options=conversion_options) + + if backend_configuration is None: + backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend) + + configure_backend(nwbfile=nwbfile_out, backend_configuration=backend_configuration) def temporally_align_data_interfaces(self): """Override this method to implement custom alignment.""" From 44d8a62e08eec11a7a36bf972831b61b667b3d47 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Fri, 17 Jan 2025 17:47:11 -0600 Subject: [PATCH 02/16] modify test --- src/neuroconv/tools/testing/data_interface_mixins.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/neuroconv/tools/testing/data_interface_mixins.py b/src/neuroconv/tools/testing/data_interface_mixins.py index d8586f44f..4f043df65 100644 --- a/src/neuroconv/tools/testing/data_interface_mixins.py +++ b/src/neuroconv/tools/testing/data_interface_mixins.py @@ -142,7 +142,6 @@ def test_run_conversion_with_backend_configuration(self, setup_interface, tmp_pa backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) self.interface.run_conversion( nwbfile_path=nwbfile_path, - nwbfile=nwbfile, overwrite=True, metadata=metadata, backend_configuration=backend_configuration, @@ -227,7 +226,6 @@ class TestNWBConverter(NWBConverter): backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) converter.run_conversion( nwbfile_path=nwbfile_path, - nwbfile=nwbfile, overwrite=True, metadata=metadata, backend_configuration=backend_configuration, From 7465000815404c9d365136c3ece703a98fc926d8 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Fri, 17 Jan 2025 19:37:41 -0600 Subject: [PATCH 03/16] fix fiber photometry I --- src/neuroconv/tools/testing/data_interface_mixins.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/neuroconv/tools/testing/data_interface_mixins.py b/src/neuroconv/tools/testing/data_interface_mixins.py index 4f043df65..f2c3be53b 100644 --- a/src/neuroconv/tools/testing/data_interface_mixins.py +++ b/src/neuroconv/tools/testing/data_interface_mixins.py @@ -1260,7 +1260,6 @@ def check_run_conversion_with_backend_configuration( backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) self.interface.run_conversion( nwbfile_path=nwbfile_path, - nwbfile=nwbfile, overwrite=True, backend_configuration=backend_configuration, **self.conversion_options, @@ -1301,7 +1300,6 @@ class TestNWBConverter(NWBConverter): backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) converter.run_conversion( nwbfile_path=nwbfile_path, - nwbfile=nwbfile, overwrite=True, metadata=metadata, backend_configuration=backend_configuration, From e3267f07b4dcb28fd007b3a186d5379332b82103 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Fri, 17 Jan 2025 21:15:06 -0600 Subject: [PATCH 04/16] fix med pc --- src/neuroconv/tools/testing/data_interface_mixins.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/neuroconv/tools/testing/data_interface_mixins.py b/src/neuroconv/tools/testing/data_interface_mixins.py index f2c3be53b..b4db720b0 100644 --- a/src/neuroconv/tools/testing/data_interface_mixins.py +++ b/src/neuroconv/tools/testing/data_interface_mixins.py @@ -887,7 +887,6 @@ def check_run_conversion_with_backend_configuration( backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) self.interface.run_conversion( nwbfile_path=nwbfile_path, - nwbfile=nwbfile, overwrite=True, metadata=metadata, backend_configuration=backend_configuration, @@ -929,7 +928,6 @@ class TestNWBConverter(NWBConverter): backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) converter.run_conversion( nwbfile_path=nwbfile_path, - nwbfile=nwbfile, overwrite=True, metadata=metadata, backend_configuration=backend_configuration, From e2692b8137e0554ba76d02d27c9b34c0818d6a15 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Mon, 20 Jan 2025 16:54:18 -0600 Subject: [PATCH 05/16] fix fiber photometry tests --- src/neuroconv/tools/testing/data_interface_mixins.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/neuroconv/tools/testing/data_interface_mixins.py b/src/neuroconv/tools/testing/data_interface_mixins.py index b4db720b0..bfba59a45 100644 --- a/src/neuroconv/tools/testing/data_interface_mixins.py +++ b/src/neuroconv/tools/testing/data_interface_mixins.py @@ -1258,6 +1258,7 @@ def check_run_conversion_with_backend_configuration( backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) self.interface.run_conversion( nwbfile_path=nwbfile_path, + metadata=metadata, overwrite=True, backend_configuration=backend_configuration, **self.conversion_options, From a78694684edaeaedbbe152d7026b77baedf24cb3 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Mon, 20 Jan 2025 16:55:32 -0600 Subject: [PATCH 06/16] changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86c51a2c7..e93593b89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,9 @@ * Added `metadata` and `conversion_options` as arguments to `NWBConverter.temporally_align_data_interfaces` [PR #1162](https://github.com/catalystneuro/neuroconv/pull/1162) ## Improvements +* Simple writing no longer uses a context manager [PR #1180](https://github.com/catalystneuro/neuroconv/pull/1180) -# v0.6.7 (January 20, 2024) +# v0.6.7 (January 20, 2025) ## Deprecations From 980ca7218007f14a078b0384dc2a6cd1aa0f1192 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Mon, 20 Jan 2025 19:25:17 -0600 Subject: [PATCH 07/16] doctest --- docs/conversion_examples_gallery/recording/openephys.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/conversion_examples_gallery/recording/openephys.rst b/docs/conversion_examples_gallery/recording/openephys.rst index a5d4e960a..bced73b7f 100644 --- a/docs/conversion_examples_gallery/recording/openephys.rst +++ b/docs/conversion_examples_gallery/recording/openephys.rst @@ -30,4 +30,3 @@ Convert OpenEphys data to NWB using :py:class:`~neuroconv.datainterfaces.ecephys >>> # Choose a path for saving the nwb file and run the conversion >>> nwbfile_path = f"{path_to_save_nwbfile}" # This should be something like: "./saved_file.nwb" >>> interface.run_conversion(nwbfile_path=nwbfile_path, metadata=metadata) - NWB file saved at ... From 52080dc99bc81621042e455d08368397735ad751 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Tue, 21 Jan 2025 09:52:27 -0600 Subject: [PATCH 08/16] improve variable naming --- src/neuroconv/basedatainterface.py | 32 +++++++++++++++++------------- src/neuroconv/nwbconverter.py | 25 ++++++++++++----------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/neuroconv/basedatainterface.py b/src/neuroconv/basedatainterface.py index bcf158bfd..c804c1342 100644 --- a/src/neuroconv/basedatainterface.py +++ b/src/neuroconv/basedatainterface.py @@ -166,11 +166,12 @@ def run_conversion( Parameters ---------- nwbfile_path : FilePathType - Path for where the data will be written or appended. + Path where the data will be written or appended. nwbfile : NWBFile, optional An in-memory NWBFile object to write to the location. metadata : dict, optional - Metadata dictionary with information used to create the NWBFile when one does not exist or overwrite=True. + Metadata dictionary with information used to create the NWBFile + when one does not exist or overwrite=True. overwrite : bool, default: False Whether to overwrite the NWBFile if one exists at the nwbfile_path. The default is False (append mode). @@ -180,28 +181,29 @@ def run_conversion( If a `backend_configuration` is specified, then the type will be auto-detected. backend_configuration : HDF5BackendConfiguration or ZarrBackendConfiguration, optional The configuration model to use when configuring the datasets for this backend. - To customize, call the `.get_default_backend_configuration(...)` method, modify the returned - BackendConfiguration object, and pass that instead. + To customize, call the `.get_default_backend_configuration(...)` method, + modify the returned BackendConfiguration object, and pass that instead. Otherwise, all datasets will use default configuration settings. """ - backend = _resolve_backend(backend, backend_configuration) - no_nwbfile_provided = nwbfile is None # Otherwise, variable reference may mutate later on inside the context + # Check if we're appending to an in-memory NWBFile + appending_to_in_memory_nwbfile = nwbfile is not None if metadata is None: metadata = self.get_metadata() + # Determine whether we're appending to a file on disk file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False - append_mode = file_initially_exists and not overwrite + appending_to_disk_nwbfile = file_initially_exists and not overwrite - self.validate_metadata(metadata=metadata, append_mode=append_mode) + self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile) - if not append_mode: - - if no_nwbfile_provided: - nwbfile = self.create_nwbfile(metadata=metadata, **conversion_options) - else: + # If we're NOT appending to an existing file on disk (i.e., new file or overwrite=True) + if not appending_to_disk_nwbfile: + if appending_to_in_memory_nwbfile: self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, **conversion_options) + else: + nwbfile = self.create_nwbfile(metadata=metadata, **conversion_options) configure_and_write_nwbfile( nwbfile=nwbfile, @@ -211,6 +213,8 @@ def run_conversion( ) else: # We are only using the context in append mode, see #1143 + backend = _resolve_backend(backend, backend_configuration) + with make_or_load_nwbfile( nwbfile_path=nwbfile_path, nwbfile=nwbfile, @@ -219,7 +223,7 @@ def run_conversion( backend=backend, verbose=getattr(self, "verbose", False), ) as nwbfile_out: - if no_nwbfile_provided: + if not appending_to_in_memory_nwbfile: self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, **conversion_options) if backend_configuration is None: diff --git a/src/neuroconv/nwbconverter.py b/src/neuroconv/nwbconverter.py index 5846feb50..48a42e8d1 100644 --- a/src/neuroconv/nwbconverter.py +++ b/src/neuroconv/nwbconverter.py @@ -244,26 +244,24 @@ def run_conversion( " use Converter.add_to_nwbfile." ) - backend = _resolve_backend(backend, backend_configuration) - no_nwbfile_provided = nwbfile is None # Otherwise, variable reference may mutate later on inside the context - - file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False - append_mode = file_initially_exists and not overwrite + appending_to_in_memory_nwbfile = nwbfile is not None if metadata is None: metadata = self.get_metadata() - self.validate_metadata(metadata=metadata, append_mode=append_mode) + file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False + appending_to_disk_nwbfile = file_initially_exists and not overwrite + self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile) self.validate_conversion_options(conversion_options=conversion_options) self.temporally_align_data_interfaces(metadata=metadata, conversion_options=conversion_options) - if not append_mode: + if not appending_to_disk_nwbfile: - if no_nwbfile_provided: - nwbfile = self.create_nwbfile(metadata=metadata, conversion_options=conversion_options) - else: + if appending_to_in_memory_nwbfile: self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, conversion_options=conversion_options) + else: + nwbfile = self.create_nwbfile(metadata=metadata, conversion_options=conversion_options) configure_and_write_nwbfile( nwbfile=nwbfile, @@ -272,7 +270,10 @@ def run_conversion( backend_configuration=backend_configuration, ) - else: # We are only using the context in append mode, see #1143 + else: # We are only using the context in append mode, see issue #1143 + + backend = _resolve_backend(backend, backend_configuration) + with make_or_load_nwbfile( nwbfile_path=nwbfile_path, nwbfile=nwbfile, @@ -281,7 +282,7 @@ def run_conversion( backend=backend, verbose=getattr(self, "verbose", False), ) as nwbfile_out: - if no_nwbfile_provided: + if not appending_to_in_memory_nwbfile: self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, conversion_options=conversion_options) if backend_configuration is None: From 1ffdb8effa7891848ba9d913e46058f821906529 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Tue, 21 Jan 2025 09:58:28 -0600 Subject: [PATCH 09/16] docstring revert --- src/neuroconv/basedatainterface.py | 20 +++++++------------- src/neuroconv/nwbconverter.py | 6 ++---- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/neuroconv/basedatainterface.py b/src/neuroconv/basedatainterface.py index c804c1342..45a76fd2c 100644 --- a/src/neuroconv/basedatainterface.py +++ b/src/neuroconv/basedatainterface.py @@ -166,12 +166,11 @@ def run_conversion( Parameters ---------- nwbfile_path : FilePathType - Path where the data will be written or appended. + Path for where to write or load (if overwrite=False) the NWBFile. nwbfile : NWBFile, optional An in-memory NWBFile object to write to the location. metadata : dict, optional - Metadata dictionary with information used to create the NWBFile - when one does not exist or overwrite=True. + Metadata dictionary with information used to create the NWBFile when one does not exist or overwrite=True. overwrite : bool, default: False Whether to overwrite the NWBFile if one exists at the nwbfile_path. The default is False (append mode). @@ -181,24 +180,19 @@ def run_conversion( If a `backend_configuration` is specified, then the type will be auto-detected. backend_configuration : HDF5BackendConfiguration or ZarrBackendConfiguration, optional The configuration model to use when configuring the datasets for this backend. - To customize, call the `.get_default_backend_configuration(...)` method, - modify the returned BackendConfiguration object, and pass that instead. + To customize, call the `.get_default_backend_configuration(...)` method, modify the returned + BackendConfiguration object, and pass that instead. Otherwise, all datasets will use default configuration settings. """ - # Check if we're appending to an in-memory NWBFile appending_to_in_memory_nwbfile = nwbfile is not None - - if metadata is None: - metadata = self.get_metadata() - - # Determine whether we're appending to a file on disk file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False appending_to_disk_nwbfile = file_initially_exists and not overwrite + if metadata is None: + metadata = self.get_metadata() self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile) - # If we're NOT appending to an existing file on disk (i.e., new file or overwrite=True) if not appending_to_disk_nwbfile: if appending_to_in_memory_nwbfile: self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, **conversion_options) @@ -212,7 +206,7 @@ def run_conversion( backend_configuration=backend_configuration, ) - else: # We are only using the context in append mode, see #1143 + else: # We are only using the context in append mode, see issue #1143 backend = _resolve_backend(backend, backend_configuration) with make_or_load_nwbfile( diff --git a/src/neuroconv/nwbconverter.py b/src/neuroconv/nwbconverter.py index 48a42e8d1..e05ebf751 100644 --- a/src/neuroconv/nwbconverter.py +++ b/src/neuroconv/nwbconverter.py @@ -245,15 +245,14 @@ def run_conversion( ) appending_to_in_memory_nwbfile = nwbfile is not None + file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False + appending_to_disk_nwbfile = file_initially_exists and not overwrite if metadata is None: metadata = self.get_metadata() - file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False - appending_to_disk_nwbfile = file_initially_exists and not overwrite self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile) self.validate_conversion_options(conversion_options=conversion_options) - self.temporally_align_data_interfaces(metadata=metadata, conversion_options=conversion_options) if not appending_to_disk_nwbfile: @@ -271,7 +270,6 @@ def run_conversion( ) else: # We are only using the context in append mode, see issue #1143 - backend = _resolve_backend(backend, backend_configuration) with make_or_load_nwbfile( From 4a7848ef44edd51627135b5b2f07a1d77821a141 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Tue, 21 Jan 2025 10:22:53 -0600 Subject: [PATCH 10/16] remove empty test --- .../tools/testing/data_interface_mixins.py | 48 ++++--------------- .../ecephys/test_recording_interfaces.py | 2 +- 2 files changed, 10 insertions(+), 40 deletions(-) diff --git a/src/neuroconv/tools/testing/data_interface_mixins.py b/src/neuroconv/tools/testing/data_interface_mixins.py index bfba59a45..e75a03a0c 100644 --- a/src/neuroconv/tools/testing/data_interface_mixins.py +++ b/src/neuroconv/tools/testing/data_interface_mixins.py @@ -131,22 +131,13 @@ def test_run_conversion_with_backend(self, setup_interface, tmp_path, backend): io.read() @pytest.mark.parametrize("backend", ["hdf5", "zarr"]) - def test_run_conversion_with_backend_configuration(self, setup_interface, tmp_path, backend): + def test_create_backend_configuration(self, setup_interface, tmp_path, backend): metadata = self.interface.get_metadata() if "session_start_time" not in metadata["NWBFile"]: metadata["NWBFile"].update(session_start_time=datetime.now().astimezone()) - nwbfile_path = str(tmp_path / f"conversion_with_backend_configuration{backend}-{self.test_name}.nwb") - nwbfile = self.interface.create_nwbfile(metadata=metadata, **self.conversion_options) backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) - self.interface.run_conversion( - nwbfile_path=nwbfile_path, - overwrite=True, - metadata=metadata, - backend_configuration=backend_configuration, - **self.conversion_options, - ) @pytest.mark.parametrize("backend", ["hdf5", "zarr"]) def test_configure_backend_for_equivalent_nwbfiles(self, setup_interface, tmp_path, backend): @@ -168,7 +159,7 @@ def test_all_conversion_checks(self, setup_interface, tmp_path): self.nwbfile_path = nwbfile_path self.check_run_conversion_in_nwbconverter_with_backend(nwbfile_path=nwbfile_path, backend="hdf5") - self.check_run_conversion_in_nwbconverter_with_backend_configuration(nwbfile_path=nwbfile_path, backend="hdf5") + self.check_create_backend_configuration_with_converter(nwbfile_path=nwbfile_path, backend="hdf5") self.check_read_nwb(nwbfile_path=nwbfile_path) @@ -207,7 +198,7 @@ class TestNWBConverter(NWBConverter): conversion_options=conversion_options, ) - def check_run_conversion_in_nwbconverter_with_backend_configuration( + def check_create_backend_configuration_with_converter( self, nwbfile_path: str, backend: Union["hdf5", "zarr"] = "hdf5" ): class TestNWBConverter(NWBConverter): @@ -224,13 +215,6 @@ class TestNWBConverter(NWBConverter): nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options) backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) - converter.run_conversion( - nwbfile_path=nwbfile_path, - overwrite=True, - metadata=metadata, - backend_configuration=backend_configuration, - conversion_options=conversion_options, - ) class TemporalAlignmentMixin: @@ -823,7 +807,7 @@ def test_metadata_schema_valid(self): def test_run_conversion_with_backend(self): pass - def test_run_conversion_with_backend_configuration(self): + def test_create_backend_configuration(self): pass def test_no_metadata_mutation(self): @@ -912,7 +896,7 @@ class TestNWBConverter(NWBConverter): conversion_options=conversion_options, ) - def check_run_conversion_in_nwbconverter_with_backend_configuration( + def check_create_backend_configuration_with_converter( self, nwbfile_path: str, metadata: dict, backend: Union["hdf5", "zarr"] = "hdf5" ): class TestNWBConverter(NWBConverter): @@ -926,13 +910,6 @@ class TestNWBConverter(NWBConverter): nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options) backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) - converter.run_conversion( - nwbfile_path=nwbfile_path, - overwrite=True, - metadata=metadata, - backend_configuration=backend_configuration, - conversion_options=conversion_options, - ) def test_all_conversion_checks(self, metadata: dict): interface_kwargs = self.interface_kwargs @@ -956,7 +933,7 @@ def test_all_conversion_checks(self, metadata: dict): self.check_run_conversion_in_nwbconverter_with_backend( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) - self.check_run_conversion_in_nwbconverter_with_backend_configuration( + self.check_create_backend_configuration_with_converter( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) @@ -1199,7 +1176,7 @@ def test_conversion_options_schema_valid(self): def test_run_conversion_with_backend(self): pass - def test_run_conversion_with_backend_configuration(self): + def test_create_backend_configuration(self): pass def test_no_metadata_mutation(self): @@ -1283,7 +1260,7 @@ class TestNWBConverter(NWBConverter): conversion_options=conversion_options, ) - def check_run_conversion_in_nwbconverter_with_backend_configuration( + def check_create_backend_configuration_with_converter( self, nwbfile_path: str, metadata: dict, backend: Union["hdf5", "zarr"] = "hdf5" ): class TestNWBConverter(NWBConverter): @@ -1297,13 +1274,6 @@ class TestNWBConverter(NWBConverter): nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options) backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) - converter.run_conversion( - nwbfile_path=nwbfile_path, - overwrite=True, - metadata=metadata, - backend_configuration=backend_configuration, - conversion_options=conversion_options, - ) def test_all_conversion_checks(self, metadata: dict): interface_kwargs = self.interface_kwargs @@ -1327,7 +1297,7 @@ def test_all_conversion_checks(self, metadata: dict): self.check_run_conversion_in_nwbconverter_with_backend( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) - self.check_run_conversion_in_nwbconverter_with_backend_configuration( + self.check_create_backend_configuration_with_converter( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) diff --git a/tests/test_on_data/ecephys/test_recording_interfaces.py b/tests/test_on_data/ecephys/test_recording_interfaces.py index e4bd126f4..6cb827cc4 100644 --- a/tests/test_on_data/ecephys/test_recording_interfaces.py +++ b/tests/test_on_data/ecephys/test_recording_interfaces.py @@ -212,7 +212,7 @@ def test_no_metadata_mutation(self): def test_run_conversion_with_backend(self): pass - def test_run_conversion_with_backend_configuration(self): + def test_create_backend_configuration(self): pass def test_interface_alignment(self): From 1a0d76777c52f41908cb044ac0070002bfcd9fd9 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Tue, 21 Jan 2025 10:57:01 -0600 Subject: [PATCH 11/16] add errors --- src/neuroconv/basedatainterface.py | 12 +++++++++--- src/neuroconv/nwbconverter.py | 11 ++++++++--- tests/test_ecephys/test_ecephys_interfaces.py | 19 +++++++++++++++++++ 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/neuroconv/basedatainterface.py b/src/neuroconv/basedatainterface.py index 45a76fd2c..c2ac9b2b2 100644 --- a/src/neuroconv/basedatainterface.py +++ b/src/neuroconv/basedatainterface.py @@ -207,8 +207,14 @@ def run_conversion( ) else: # We are only using the context in append mode, see issue #1143 - backend = _resolve_backend(backend, backend_configuration) + if appending_to_disk_nwbfile: + raise ValueError( + "Cannot append to an existing file while also providing an in-memory NWBFile. " + "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." + ) + + backend = _resolve_backend(backend, backend_configuration) with make_or_load_nwbfile( nwbfile_path=nwbfile_path, nwbfile=nwbfile, @@ -217,8 +223,8 @@ def run_conversion( backend=backend, verbose=getattr(self, "verbose", False), ) as nwbfile_out: - if not appending_to_in_memory_nwbfile: - self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, **conversion_options) + + self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, **conversion_options) if backend_configuration is None: backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend) diff --git a/src/neuroconv/nwbconverter.py b/src/neuroconv/nwbconverter.py index e05ebf751..a289157dc 100644 --- a/src/neuroconv/nwbconverter.py +++ b/src/neuroconv/nwbconverter.py @@ -270,8 +270,14 @@ def run_conversion( ) else: # We are only using the context in append mode, see issue #1143 - backend = _resolve_backend(backend, backend_configuration) + if appending_to_disk_nwbfile: + raise ValueError( + "Cannot append to an existing file while also providing an in-memory NWBFile. " + "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." + ) + + backend = _resolve_backend(backend, backend_configuration) with make_or_load_nwbfile( nwbfile_path=nwbfile_path, nwbfile=nwbfile, @@ -280,8 +286,7 @@ def run_conversion( backend=backend, verbose=getattr(self, "verbose", False), ) as nwbfile_out: - if not appending_to_in_memory_nwbfile: - self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, conversion_options=conversion_options) + self.add_to_nwbfile(nwbfile=nwbfile_out, metadata=metadata, conversion_options=conversion_options) if backend_configuration is None: backend_configuration = self.get_default_backend_configuration(nwbfile=nwbfile_out, backend=backend) diff --git a/tests/test_ecephys/test_ecephys_interfaces.py b/tests/test_ecephys/test_ecephys_interfaces.py index d2fdd68ba..8a32abcac 100644 --- a/tests/test_ecephys/test_ecephys_interfaces.py +++ b/tests/test_ecephys/test_ecephys_interfaces.py @@ -6,6 +6,7 @@ from hdmf.testing import TestCase from packaging.version import Version +from neuroconv import ConverterPipe from neuroconv.datainterfaces import Spike2RecordingInterface from neuroconv.tools.nwb_helpers import get_module from neuroconv.tools.testing.mock_interfaces import ( @@ -158,6 +159,24 @@ def test_group_naming_not_adding_extra_devices(self, setup_interface): assert len(nwbfile.devices) == 1 assert len(nwbfile.electrode_groups) == 4 + def test_error_for_append_with_in_memory_file(self, setup_interface, tmp_path): + + nwbfile_path = tmp_path / "test.nwb" + self.interface.run_conversion(nwbfile_path=nwbfile_path) + + nwbfile = self.interface.create_nwbfile() + + expected_error_message = ( + "Cannot append to an existing file while also providing an in-memory NWBFile. " + "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." + ) + with pytest.raises(ValueError, match=expected_error_message): + self.interface.run_conversion(nwbfile=nwbfile, nwbfile_path=nwbfile_path, overwrite=False) + + converter = ConverterPipe(data_interfaces=[self.interface]) + with pytest.raises(ValueError, match=expected_error_message): + converter.run_conversion(nwbfile=nwbfile, nwbfile_path=nwbfile_path, overwrite=False) + class TestAssertions(TestCase): @pytest.mark.skipif(python_version.minor != 10, reason="Only testing with Python 3.10!") From 97a7ff1de29bf739a98d010782ceb754d30e7c71 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Wed, 22 Jan 2025 16:29:45 -0600 Subject: [PATCH 12/16] use correct fixes --- src/neuroconv/basedatainterface.py | 2 +- src/neuroconv/nwbconverter.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/neuroconv/basedatainterface.py b/src/neuroconv/basedatainterface.py index c2ac9b2b2..ba021c119 100644 --- a/src/neuroconv/basedatainterface.py +++ b/src/neuroconv/basedatainterface.py @@ -208,7 +208,7 @@ def run_conversion( else: # We are only using the context in append mode, see issue #1143 - if appending_to_disk_nwbfile: + if appending_to_in_memory_nwbfile: raise ValueError( "Cannot append to an existing file while also providing an in-memory NWBFile. " "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." diff --git a/src/neuroconv/nwbconverter.py b/src/neuroconv/nwbconverter.py index a289157dc..90b86230f 100644 --- a/src/neuroconv/nwbconverter.py +++ b/src/neuroconv/nwbconverter.py @@ -271,7 +271,7 @@ def run_conversion( else: # We are only using the context in append mode, see issue #1143 - if appending_to_disk_nwbfile: + if appending_to_in_memory_nwbfile: raise ValueError( "Cannot append to an existing file while also providing an in-memory NWBFile. " "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." From 4cfd6035dd156849ec44430b5a2aad37af26c420 Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Thu, 23 Jan 2025 10:47:34 -0600 Subject: [PATCH 13/16] revert test change --- .../tools/testing/data_interface_mixins.py | 48 +++++++++++++++---- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/src/neuroconv/tools/testing/data_interface_mixins.py b/src/neuroconv/tools/testing/data_interface_mixins.py index e75a03a0c..bfba59a45 100644 --- a/src/neuroconv/tools/testing/data_interface_mixins.py +++ b/src/neuroconv/tools/testing/data_interface_mixins.py @@ -131,13 +131,22 @@ def test_run_conversion_with_backend(self, setup_interface, tmp_path, backend): io.read() @pytest.mark.parametrize("backend", ["hdf5", "zarr"]) - def test_create_backend_configuration(self, setup_interface, tmp_path, backend): + def test_run_conversion_with_backend_configuration(self, setup_interface, tmp_path, backend): metadata = self.interface.get_metadata() if "session_start_time" not in metadata["NWBFile"]: metadata["NWBFile"].update(session_start_time=datetime.now().astimezone()) + nwbfile_path = str(tmp_path / f"conversion_with_backend_configuration{backend}-{self.test_name}.nwb") + nwbfile = self.interface.create_nwbfile(metadata=metadata, **self.conversion_options) backend_configuration = self.interface.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) + self.interface.run_conversion( + nwbfile_path=nwbfile_path, + overwrite=True, + metadata=metadata, + backend_configuration=backend_configuration, + **self.conversion_options, + ) @pytest.mark.parametrize("backend", ["hdf5", "zarr"]) def test_configure_backend_for_equivalent_nwbfiles(self, setup_interface, tmp_path, backend): @@ -159,7 +168,7 @@ def test_all_conversion_checks(self, setup_interface, tmp_path): self.nwbfile_path = nwbfile_path self.check_run_conversion_in_nwbconverter_with_backend(nwbfile_path=nwbfile_path, backend="hdf5") - self.check_create_backend_configuration_with_converter(nwbfile_path=nwbfile_path, backend="hdf5") + self.check_run_conversion_in_nwbconverter_with_backend_configuration(nwbfile_path=nwbfile_path, backend="hdf5") self.check_read_nwb(nwbfile_path=nwbfile_path) @@ -198,7 +207,7 @@ class TestNWBConverter(NWBConverter): conversion_options=conversion_options, ) - def check_create_backend_configuration_with_converter( + def check_run_conversion_in_nwbconverter_with_backend_configuration( self, nwbfile_path: str, backend: Union["hdf5", "zarr"] = "hdf5" ): class TestNWBConverter(NWBConverter): @@ -215,6 +224,13 @@ class TestNWBConverter(NWBConverter): nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options) backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) + converter.run_conversion( + nwbfile_path=nwbfile_path, + overwrite=True, + metadata=metadata, + backend_configuration=backend_configuration, + conversion_options=conversion_options, + ) class TemporalAlignmentMixin: @@ -807,7 +823,7 @@ def test_metadata_schema_valid(self): def test_run_conversion_with_backend(self): pass - def test_create_backend_configuration(self): + def test_run_conversion_with_backend_configuration(self): pass def test_no_metadata_mutation(self): @@ -896,7 +912,7 @@ class TestNWBConverter(NWBConverter): conversion_options=conversion_options, ) - def check_create_backend_configuration_with_converter( + def check_run_conversion_in_nwbconverter_with_backend_configuration( self, nwbfile_path: str, metadata: dict, backend: Union["hdf5", "zarr"] = "hdf5" ): class TestNWBConverter(NWBConverter): @@ -910,6 +926,13 @@ class TestNWBConverter(NWBConverter): nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options) backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) + converter.run_conversion( + nwbfile_path=nwbfile_path, + overwrite=True, + metadata=metadata, + backend_configuration=backend_configuration, + conversion_options=conversion_options, + ) def test_all_conversion_checks(self, metadata: dict): interface_kwargs = self.interface_kwargs @@ -933,7 +956,7 @@ def test_all_conversion_checks(self, metadata: dict): self.check_run_conversion_in_nwbconverter_with_backend( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) - self.check_create_backend_configuration_with_converter( + self.check_run_conversion_in_nwbconverter_with_backend_configuration( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) @@ -1176,7 +1199,7 @@ def test_conversion_options_schema_valid(self): def test_run_conversion_with_backend(self): pass - def test_create_backend_configuration(self): + def test_run_conversion_with_backend_configuration(self): pass def test_no_metadata_mutation(self): @@ -1260,7 +1283,7 @@ class TestNWBConverter(NWBConverter): conversion_options=conversion_options, ) - def check_create_backend_configuration_with_converter( + def check_run_conversion_in_nwbconverter_with_backend_configuration( self, nwbfile_path: str, metadata: dict, backend: Union["hdf5", "zarr"] = "hdf5" ): class TestNWBConverter(NWBConverter): @@ -1274,6 +1297,13 @@ class TestNWBConverter(NWBConverter): nwbfile = converter.create_nwbfile(metadata=metadata, conversion_options=conversion_options) backend_configuration = converter.get_default_backend_configuration(nwbfile=nwbfile, backend=backend) + converter.run_conversion( + nwbfile_path=nwbfile_path, + overwrite=True, + metadata=metadata, + backend_configuration=backend_configuration, + conversion_options=conversion_options, + ) def test_all_conversion_checks(self, metadata: dict): interface_kwargs = self.interface_kwargs @@ -1297,7 +1327,7 @@ def test_all_conversion_checks(self, metadata: dict): self.check_run_conversion_in_nwbconverter_with_backend( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) - self.check_create_backend_configuration_with_converter( + self.check_run_conversion_in_nwbconverter_with_backend_configuration( nwbfile_path=self.nwbfile_path, metadata=metadata, backend="hdf5" ) From c0f02f81353bf3c9755718603ec9876c1fb6f23f Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Thu, 23 Jan 2025 11:56:32 -0600 Subject: [PATCH 14/16] variable naming --- src/neuroconv/basedatainterface.py | 6 +++--- src/neuroconv/nwbconverter.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/neuroconv/basedatainterface.py b/src/neuroconv/basedatainterface.py index ba021c119..557f9548c 100644 --- a/src/neuroconv/basedatainterface.py +++ b/src/neuroconv/basedatainterface.py @@ -187,13 +187,13 @@ def run_conversion( appending_to_in_memory_nwbfile = nwbfile is not None file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False - appending_to_disk_nwbfile = file_initially_exists and not overwrite + appending_to_in_disk_nwbfile = file_initially_exists and not overwrite if metadata is None: metadata = self.get_metadata() - self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile) + self.validate_metadata(metadata=metadata, append_mode=appending_to_in_disk_nwbfile) - if not appending_to_disk_nwbfile: + if not appending_to_in_disk_nwbfile: if appending_to_in_memory_nwbfile: self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, **conversion_options) else: diff --git a/src/neuroconv/nwbconverter.py b/src/neuroconv/nwbconverter.py index 6a3a1e1c0..2782a6cac 100644 --- a/src/neuroconv/nwbconverter.py +++ b/src/neuroconv/nwbconverter.py @@ -246,16 +246,16 @@ def run_conversion( appending_to_in_memory_nwbfile = nwbfile is not None file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False - appending_to_disk_nwbfile = file_initially_exists and not overwrite + appending_to_in_disk_nwbfile = file_initially_exists and not overwrite if metadata is None: metadata = self.get_metadata() - self.validate_metadata(metadata=metadata, append_mode=appending_to_disk_nwbfile) + self.validate_metadata(metadata=metadata, append_mode=appending_to_in_disk_nwbfile) self.validate_conversion_options(conversion_options=conversion_options) self.temporally_align_data_interfaces(metadata=metadata, conversion_options=conversion_options) - if not appending_to_disk_nwbfile: + if not appending_to_in_disk_nwbfile: if appending_to_in_memory_nwbfile: self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, conversion_options=conversion_options) From 4c40aa12397ecd1db64d1a13812e3e66610ccc2c Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Thu, 23 Jan 2025 12:10:04 -0600 Subject: [PATCH 15/16] revert another test --- tests/test_on_data/ecephys/test_recording_interfaces.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_on_data/ecephys/test_recording_interfaces.py b/tests/test_on_data/ecephys/test_recording_interfaces.py index 6cb827cc4..e4bd126f4 100644 --- a/tests/test_on_data/ecephys/test_recording_interfaces.py +++ b/tests/test_on_data/ecephys/test_recording_interfaces.py @@ -212,7 +212,7 @@ def test_no_metadata_mutation(self): def test_run_conversion_with_backend(self): pass - def test_create_backend_configuration(self): + def test_run_conversion_with_backend_configuration(self): pass def test_interface_alignment(self): From 9397ab638a86ccee8e1e399adb3e6bc7fa66544e Mon Sep 17 00:00:00 2001 From: Heberto Mayorquin Date: Thu, 23 Jan 2025 12:31:29 -0600 Subject: [PATCH 16/16] move error to start --- src/neuroconv/basedatainterface.py | 12 ++++++------ src/neuroconv/nwbconverter.py | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/neuroconv/basedatainterface.py b/src/neuroconv/basedatainterface.py index 557f9548c..11a07f7b6 100644 --- a/src/neuroconv/basedatainterface.py +++ b/src/neuroconv/basedatainterface.py @@ -189,6 +189,12 @@ def run_conversion( file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False appending_to_in_disk_nwbfile = file_initially_exists and not overwrite + if appending_to_in_disk_nwbfile and appending_to_in_memory_nwbfile: + raise ValueError( + "Cannot append to an existing file while also providing an in-memory NWBFile. " + "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." + ) + if metadata is None: metadata = self.get_metadata() self.validate_metadata(metadata=metadata, append_mode=appending_to_in_disk_nwbfile) @@ -208,12 +214,6 @@ def run_conversion( else: # We are only using the context in append mode, see issue #1143 - if appending_to_in_memory_nwbfile: - raise ValueError( - "Cannot append to an existing file while also providing an in-memory NWBFile. " - "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." - ) - backend = _resolve_backend(backend, backend_configuration) with make_or_load_nwbfile( nwbfile_path=nwbfile_path, diff --git a/src/neuroconv/nwbconverter.py b/src/neuroconv/nwbconverter.py index 2782a6cac..05e26e866 100644 --- a/src/neuroconv/nwbconverter.py +++ b/src/neuroconv/nwbconverter.py @@ -248,6 +248,12 @@ def run_conversion( file_initially_exists = Path(nwbfile_path).exists() if nwbfile_path is not None else False appending_to_in_disk_nwbfile = file_initially_exists and not overwrite + if appending_to_in_disk_nwbfile and appending_to_in_memory_nwbfile: + raise ValueError( + "Cannot append to an existing file while also providing an in-memory NWBFile. " + "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." + ) + if metadata is None: metadata = self.get_metadata() @@ -271,12 +277,6 @@ def run_conversion( else: # We are only using the context in append mode, see issue #1143 - if appending_to_in_memory_nwbfile: - raise ValueError( - "Cannot append to an existing file while also providing an in-memory NWBFile. " - "Either set overwrite=True to replace the existing file, or remove the nwbfile parameter to append to the existing file on disk." - ) - backend = _resolve_backend(backend, backend_configuration) with make_or_load_nwbfile( nwbfile_path=nwbfile_path,