NeurodataWithoutBorders / matnwb

A Matlab interface for reading and writing NWB files
BSD 2-Clause "Simplified" License
49 stars 32 forks source link

Allow unit vectordata #506

Closed lawrence-mbf closed 1 year ago

lawrence-mbf commented 1 year ago

Fixes #238

Motivation

See #238 and friends

How to test the behavior?

See #238 and friends. A good example file is DANDIset 000022

Checklist

rly commented 1 year ago

Will you add the same workaround for the sampling_rate attribute on the "waveform_mean", "waveform_sd", and "waveforms" named VectorData, and the resolution attribute on the "spike_times" named VectorData?

https://github.com/NeurodataWithoutBorders/nwb-schema/blob/dev/core/nwb.misc.yaml#L248 https://github.com/NeurodataWithoutBorders/nwb-schema/blob/dev/core/nwb.misc.yaml#L193

lawrence-mbf commented 1 year ago

Will you add the same workaround for the sampling_rate attribute on the "waveform_mean", "waveform_sd", and "waveforms" named VectorData, and the resolution attribute on the "spike_times" named VectorData?

https://github.com/NeurodataWithoutBorders/nwb-schema/blob/dev/core/nwb.misc.yaml#L248 https://github.com/NeurodataWithoutBorders/nwb-schema/blob/dev/core/nwb.misc.yaml#L193

I can do that but could either @rly or @bendichter draft a python script that creates a Unit table with these attributes so I can add it to the python tests?

codecov[bot] commented 1 year ago

Codecov Report

Merging #506 (235c16f) into master (442c604) will increase coverage by 0.30%. The diff coverage is 94.65%.

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
+ Coverage   87.58%   87.89%   +0.30%     
==========================================
  Files         129      129              
  Lines        5340     5451     +111     
==========================================
+ Hits         4677     4791     +114     
+ Misses        663      660       -3     
Impacted Files Coverage Δ
+io/writeCompound.m 77.33% <77.02%> (ø)
NwbFile.m 82.01% <88.37%> (ø)
+file/processClass.m 93.65% <93.65%> (+6.55%) :arrow_up:
+file/fillExport.m 98.68% <99.33%> (+0.21%) :arrow_up:
+file/fillClass.m 100.00% <100.00%> (ø)
+file/fillProps.m 95.89% <100.00%> (+0.05%) :arrow_up:
+file/fillSetters.m 100.00% <100.00%> (ø)
+tests/+system/UnitTimesIOTest.m 100.00% <100.00%> (ø)
+tests/+util/verifyContainerEqual.m 100.00% <100.00%> (+1.96%) :arrow_up:
+types/+untyped/DataStub.m 95.06% <100.00%> (+0.02%) :arrow_up:
... and 2 more

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

bendichter commented 1 year ago

I did a simple spot check on this dataset. Read was broken before and with this change read is fixed. I'll try a few more spot checks.

bendichter commented 1 year ago

@yarikoptic would it be possible to run the healthstatus checks against this branch? It appears to work but I want to be thorough

yarikoptic commented 1 year ago

not "easily". We have https://github.com/dandi/dandisets-healthstatus/issues/20 but not yet pursued that direction. and unfortunately currently running healthchecks, especially matlab one, is way too slow. heh. But if you know which dandiset/files you would like to run them on, you could even use this KISS helper https://github.com/dandi/dandisets-healthstatus/blob/main/code/checknwb.sh - just tune to your branch etc.

bendichter commented 1 year ago

Ueli's lab has confirmed that this works for them, fixing a write command that uses waveform_mean.sampling_rate and waveform_mean.unit