[Backend Configuration IIIb] Configure DynamicTable router for set_data_io #700

CodyCBakerPhD · 2024-01-02T15:56:01Z

Adds support for dynamic tables to the backend configuration tool, which previously had a bug related to hdmf-dev/hdmf#1013

@alejoe91 Do note that when testing this, you will need to use HDMF dev (pip install git+https://github.com/hdmf-dev/hdmf.git@dev or fresh pip install -e on this branch)

…test on dynamic tables

for more information, see https://pre-commit.ci

…neuroconv into set_data_io_debug

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py

for more information, see https://pre-commit.ci

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py

Co-authored-by: Alessio Buccino <[email protected]>

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py

for more information, see https://pre-commit.ci

alejoe91 · 2024-01-03T15:51:53Z

Testing now @CodyCBakerPhD !!! :)

h-mayorquin

This looks good to me. Some small comments that are questions and suggestions (all non-blocking for me).

One big point though is: are we pinning to a hdmf dev branch? What's the justication of this? is this needed with urgency?

requirements-minimal.txt

src/neuroconv/tools/nwb_helpers/_configure_backend.py

h-mayorquin · 2024-01-04T13:33:46Z

src/neuroconv/tools/nwb_helpers/_configure_backend.py

-        nwbfile_objects[object_id].set_data_io(dataset_name=dataset_name, data_io_class=data_io_class, **data_io_kwargs)
+        if isinstance(nwbfile_objects[object_id], Data):
+            nwbfile_objects[object_id].set_data_io(data_io_class=data_io_class, data_io_kwargs=data_io_kwargs)
+        elif isinstance(nwbfile_objects[object_id], TimeSeries):


What happens with the timestamps if they are present? Are they chunked compressed?

Ought to be

Are they Data or a TimeSeries? I guess that was my question.

They are a dataset of the TimeSeries

(it would enter line 29 here with dataset_name="timestamps")

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py

h-mayorquin · 2024-01-04T13:41:04Z

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py

+    configure_backend(nwbfile=nwbfile, backend_configuration=backend_configuration)
+
+    nwbfile_path = str(tmpdir / f"test_configure_defaults_dynamic_table.nwb.{backend}")
+    with BACKEND_NWB_IO[backend](path=nwbfile_path, mode="w") as io:


I would also avoid inlining the IO class here. I think it makes the code harder to read but is marginal.

h-mayorquin · 2024-01-04T13:44:04Z

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py

Maybe we could add a ragged array tests. Does chunking makes sense on those cases? I guess it is very difficult to define a good chunking pattern unlesss your entries are homogeneous.

It would; I think we do have general configuration tests for this, just not for the configure_backend call since in principle there's nothing at all different about those special columns that have _index in the name (they are still just VectorData at the end of the day)

CodyCBakerPhD · 2024-01-04T15:54:58Z

What's the justication of this? is this needed with urgency?

The urgency is this is a bug Alessio found when trying out the tools; unable to write something on the electrodes table

Granted there are other ways to disable that detection (on our side, or the users), but it's an intended thing for this feature to support so we should fix it as quickly as we can

h-mayorquin · 2024-01-04T16:18:26Z

The urgency is this is a bug Alessio found when trying out the tools; unable to write something on the electrodes table

All right, fine by me to merge. I am not that concerned about pinning to a specific branch or commit outside of a release.

I guess that @alejoe91 will test this so he can approve it once it works.

src/neuroconv/tools/nwb_helpers/_configure_backend.py

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py

src/neuroconv/tools/nwb_helpers/_configure_backend.py

for more information, see https://pre-commit.ci

CodyCBakerPhD · 2024-01-04T16:48:01Z

@alejoe91 If you can confirm that this allows you to proceed testing configure_backend around that electrodes table issue (but separate from the writing zeros issue, which will be fixed in a follow-up)

codecov · 2024-01-04T17:02:34Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1ddfe22) 92.20% compared to head (1abca9e) 92.20%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #700      +/-   ##
==========================================
- Coverage   92.20%   92.20%   -0.01%     
==========================================
  Files         115      115              
  Lines        5954     5964      +10     
==========================================
+ Hits         5490     5499       +9     
- Misses        464      465       +1

Flag	Coverage Δ
unittests	`92.20% <91.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/neuroconv/tools/nwb_helpers/__init__.py	`100.00% <100.00%> (ø)`
...c/neuroconv/tools/spikeinterface/spikeinterface.py	`92.76% <100.00%> (+0.01%)`	⬆️
.../neuroconv/tools/nwb_helpers/_configure_backend.py	`94.44% <87.50%> (-5.56%)`	⬇️

alejoe91 · 2024-01-04T17:25:03Z

@alejoe91 If you can confirm that this allows you to proceed testing configure_backend around that electrodes table issue (but separate from the writing zeros issue, which will be fixed in a follow-up)

confirmed

CodyCBakerPhD · 2024-01-04T18:00:27Z

confirmed

@alejoe91 Sorry, 'confirm by approving' as per #700 (comment)

h-mayorquin · 2024-01-04T18:06:57Z

Thanks for working on this @CodyCBakerPhD. And lots of love for dealing with questions as usual!

debug by adding object type router with call sign adjustments; added …

bc9d58b

…test on dynamic tables

CodyCBakerPhD requested review from h-mayorquin and alejoe91 January 2, 2024 15:56

CodyCBakerPhD self-assigned this Jan 2, 2024

pre-commit-ci bot and others added 5 commits January 2, 2024 15:56

[pre-commit.ci] auto fixes from pre-commit.com hooks

4277785

for more information, see https://pre-commit.ci

Merge branch 'main' into set_data_io_debug

b2eb07b

fix requirement URL

d9fe5d0

Merge branch 'set_data_io_debug' of https://github.com/catalystneuro/…

8e26844

…neuroconv into set_data_io_debug

add changelog

b5aa9a2

CodyCBakerPhD commented Jan 2, 2024

View reviewed changes

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py Outdated Show resolved Hide resolved

CodyCBakerPhD commented Jan 2, 2024

View reviewed changes

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py Outdated Show resolved Hide resolved

don't need iterator parametrizations

662b5cb

CodyCBakerPhD commented Jan 2, 2024

View reviewed changes

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py Outdated Show resolved Hide resolved

CodyCBakerPhD commented Jan 2, 2024

View reviewed changes

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py Outdated Show resolved Hide resolved

CodyCBakerPhD and others added 2 commits January 2, 2024 16:16

don't need iterator options

4a07c5a

[pre-commit.ci] auto fixes from pre-commit.com hooks

37a6dac

for more information, see https://pre-commit.ci

CodyCBakerPhD commented Jan 2, 2024

View reviewed changes

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py Outdated Show resolved Hide resolved

don't need buffer arguments

1f528c7

CodyCBakerPhD enabled auto-merge (squash) January 2, 2024 21:18

alejoe91 reviewed Jan 3, 2024

View reviewed changes

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py Outdated Show resolved Hide resolved

alejoe91 reviewed Jan 3, 2024

View reviewed changes

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py Outdated Show resolved Hide resolved

CodyCBakerPhD commented Jan 3, 2024

View reviewed changes

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py Outdated Show resolved Hide resolved

CodyCBakerPhD commented Jan 3, 2024

View reviewed changes

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py Outdated Show resolved Hide resolved

remove previous arg

489a294

Co-authored-by: Alessio Buccino <[email protected]>

CodyCBakerPhD commented Jan 3, 2024

View reviewed changes

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py Outdated Show resolved Hide resolved

CodyCBakerPhD and others added 2 commits January 3, 2024 10:31

unused assert

3fe013f

[pre-commit.ci] auto fixes from pre-commit.com hooks

632aaaf

for more information, see https://pre-commit.ci

CodyCBakerPhD requested a review from alejoe91 January 3, 2024 15:37

h-mayorquin reviewed Jan 4, 2024

View reviewed changes

h-mayorquin disabled auto-merge January 4, 2024 13:47

CodyCBakerPhD commented Jan 4, 2024

View reviewed changes

src/neuroconv/tools/nwb_helpers/_configure_backend.py Outdated Show resolved Hide resolved

Update src/neuroconv/tools/nwb_helpers/_configure_backend.py

1d9c234

CodyCBakerPhD commented Jan 4, 2024

View reviewed changes

...tools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_defaults.py Outdated Show resolved Hide resolved

CodyCBakerPhD commented Jan 4, 2024

View reviewed changes

...ools/test_backend_and_dataset_configuration/test_helpers/test_configure_backend_overrides.py Outdated Show resolved Hide resolved

Apply suggestions from code review

36cf1cf

CodyCBakerPhD commented Jan 4, 2024

View reviewed changes

src/neuroconv/tools/nwb_helpers/_configure_backend.py Outdated Show resolved Hide resolved

CodyCBakerPhD and others added 2 commits January 4, 2024 11:46

indent

7a2640a

[pre-commit.ci] auto fixes from pre-commit.com hooks

1abca9e

for more information, see https://pre-commit.ci

CodyCBakerPhD enabled auto-merge (squash) January 4, 2024 16:47

alejoe91 approved these changes Jan 4, 2024

View reviewed changes

CodyCBakerPhD merged commit 4bef01d into main Jan 4, 2024
36 checks passed

CodyCBakerPhD deleted the set_data_io_debug branch January 4, 2024 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backend Configuration IIIb] Configure DynamicTable router for set_data_io #700

[Backend Configuration IIIb] Configure DynamicTable router for set_data_io #700

CodyCBakerPhD commented Jan 2, 2024

alejoe91 commented Jan 3, 2024

h-mayorquin left a comment •

edited

Loading

h-mayorquin Jan 4, 2024

CodyCBakerPhD Jan 4, 2024

h-mayorquin Jan 4, 2024

CodyCBakerPhD Jan 4, 2024

CodyCBakerPhD Jan 4, 2024

h-mayorquin Jan 4, 2024

CodyCBakerPhD Jan 4, 2024

h-mayorquin Jan 4, 2024

CodyCBakerPhD Jan 4, 2024

CodyCBakerPhD commented Jan 4, 2024

h-mayorquin commented Jan 4, 2024

CodyCBakerPhD commented Jan 4, 2024

codecov bot commented Jan 4, 2024

alejoe91 commented Jan 4, 2024

CodyCBakerPhD commented Jan 4, 2024

h-mayorquin commented Jan 4, 2024

[Backend Configuration IIIb] Configure DynamicTable router for set_data_io #700

[Backend Configuration IIIb] Configure DynamicTable router for set_data_io #700

Conversation

CodyCBakerPhD commented Jan 2, 2024

alejoe91 commented Jan 3, 2024

h-mayorquin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodyCBakerPhD commented Jan 4, 2024

h-mayorquin commented Jan 4, 2024

CodyCBakerPhD commented Jan 4, 2024

codecov bot commented Jan 4, 2024

Codecov Report

alejoe91 commented Jan 4, 2024

CodyCBakerPhD commented Jan 4, 2024

h-mayorquin commented Jan 4, 2024

h-mayorquin left a comment •

edited

Loading