catalystneuro / nwb-conversion-tools

Create NWB files by converting and combining neural data in proprietary formats and adding essential metadata.
https://nwb-conversion-tools.readthedocs.io/en/main/
BSD 3-Clause "New" or "Revised" License
25 stars 12 forks source link

Refactor metadata extraction for `ScanImageImagingInterface` #580

Closed h-mayorquin closed 2 years ago

h-mayorquin commented 2 years ago

I am trying to add the gin test for ScanImageImagingInterface #568

However, the metadata that we have right for ScanImageImagingInterface does not follow the the right format (the problem is that our format expected lists for imaging plane and two photon series and Ben added the metadata as dicts) and therefore the metadata validation fails.

The PR with the added test comes chained to this.

As a bonus, one of the warnings that was generated by improper use of dict_deep_update is removed

CodyCBakerPhD commented 2 years ago

retriggering tests

CodyCBakerPhD commented 2 years ago

LGTM - @h-mayorquin is this good to go?

codecov[bot] commented 2 years ago

Codecov Report

Merging #580 (9dc42a0) into main (856283e) will decrease coverage by 0.06%. The diff coverage is 90.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #580      +/-   ##
==========================================
- Coverage   88.40%   88.34%   -0.07%     
==========================================
  Files          59       59              
  Lines        3201     3218      +17     
==========================================
+ Hits         2830     2843      +13     
- Misses        371      375       +4     
Flag Coverage Δ
unittests 88.34% <90.32%> (-0.07%) :arrow_down:

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

Impacted Files Coverage Δ
...faces/ophys/scanimage/scanimageimaginginterface.py 88.37% <90.32%> (-0.09%) :arrow_down:
src/nwb_conversion_tools/utils/dict.py 91.80% <0.00%> (-3.28%) :arrow_down:
h-mayorquin commented 2 years ago

LGTM - @h-mayorquin is this good to go?

Yes.