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

Allow unit vectordata #506

Merged
merged 17 commits into from
May 26, 2023
Merged

Allow unit vectordata #506

merged 17 commits into from
May 26, 2023

Conversation

lawrence-mbf
Copy link
Collaborator

@lawrence-mbf lawrence-mbf commented May 17, 2023

Fixes #238

Motivation

See #238 and friends

How to test the behavior?

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

  • waveform_mean, waveform_sd, and waveforms attributes:
    • unit
    • sampling_rate
  • spike_times attributes:
    • resolution

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

- renamed variables to for clarity
- retabbed function files to include one scope for base/private functions.
If the object's data is not a reference but has region/object references in its attributes, then
this error may occur because all reference corrections require calling `export` again.

Since we want to limit the possibility of object reference cycles, it's better to write any dataset
given that it's not reference data and just check for duplicates before doing so.
Used to error, now short-circuits the output.
Removed the data-class setter format and use the correct handle-class one (without returning self).
For hard-coding VectorData.unit.
Also retabbed to maintain consistent scope for functions.
Now that we have hidden properties in our generated classes.
@lawrence-mbf lawrence-mbf added category: bug errors in the code or code behavior priority: critical impacts proper operation or use of core function of NWB or the software labels May 17, 2023
@lawrence-mbf lawrence-mbf requested review from rly and bendichter May 17, 2023 17:26
@lawrence-mbf lawrence-mbf self-assigned this May 17, 2023
@lawrence-mbf lawrence-mbf removed the request for review from rly May 17, 2023 17:28
@rly
Copy link
Contributor

rly commented May 17, 2023

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 lawrence-mbf requested a review from rly May 17, 2023 17:37
@lawrence-mbf
Copy link
Collaborator Author

lawrence-mbf commented May 17, 2023

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?

Originally applied to all export calls. This may cause issues for datapipes and simply rewriting
data so the check is now applied to datastubs instead.
@codecov
Copy link

codecov bot commented May 17, 2023

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%) ⬆️
+file/fillExport.m 98.68% <99.33%> (+0.21%) ⬆️
+file/fillClass.m 100.00% <100.00%> (ø)
+file/fillProps.m 95.89% <100.00%> (+0.05%) ⬆️
+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%) ⬆️
+types/+untyped/DataStub.m 95.06% <100.00%> (+0.02%) ⬆️
... and 2 more

... and 2 files with indirect coverage changes

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

@bendichter
Copy link
Contributor

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

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

@yarikoptic
Copy link

not "easily". We have dandi/dandisets-healthstatus#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
Copy link
Contributor

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

Currently throws a strange error when testing out to pynwb
bendichter
bendichter previously approved these changes May 24, 2023
Mostly description and default columns were missing. These have been filled in.

The getContainer() function was also changed so as to test the entire Units table and not just
one column.
@lawrence-mbf lawrence-mbf marked this pull request as ready for review May 24, 2023 17:52
@lawrence-mbf lawrence-mbf requested a review from bendichter May 24, 2023 17:52
@lawrence-mbf lawrence-mbf merged commit 43cdffb into master May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior priority: critical impacts proper operation or use of core function of NWB or the software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error reading Neuropixel file
4 participants