Updated model fields to the latest BEP32 state #278
Conversation
CodyCBakerPhD
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
==========================================
+ Coverage 84.12% 84.48% +0.35%
==========================================
Files 36 36
Lines 1392 1424 +32
==========================================
+ Hits 1171 1203 +32
Misses 221 221
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
CodyCBakerPhD
changed the title
|
After this is merged I'll get started on the conversion gallery to give side-by-side comparison to BEP32 examples |
|
@asmacdo If you could look at this when you get a chance - towards the goal of restoring validation % to BIDS-Dandisets |
asmacdo
left a comment
asmacdo
left a comment
There was a problem hiding this comment.
I compared this with the BEP32 PR spec and it mostly looks good.
time_reference_channel is the only comment that I'd consider blocking.
Also, I like the comments with units, maybe some others could benefit as well? I'll leave it to your judgement if any of these are actually useful.
_channels.py:
low_cutoff-# in Hzhigh_cutoff-# in Hzsampling_frequency-# in Hztime_offset-# in seconds
_electrodes.py:
x-# in um (probe-relative) or coordsystem unitsy-# in um (probe-relative) or coordsystem unitsz-# in um (probe-relative) or coordsystem units
_probes.py:
AP-# in mmML-# in mmDV-# in mmAP_angle-# in degreesML_angle-# in degreesdepth-# in mmrotation_angle-# in degrees
| channel_name=( | ||
| f"ch{channel_name.values[0]}" | ||
| name=( | ||
| f"ch{channel_name.values[0].zfill(3)}" |
There was a problem hiding this comment.
Probably pedantic, but if there were a non-numeric channel name, should we still do this? ie "A1" -> "ch0A1"? Should we validate that its only digits? And is 3 digits enough to be future proof?
There was a problem hiding this comment.
Fair enough in the first case of the ternary
The second case however, is guaranteed to be an integer, and zfill 3 simply matches the convention of the BEP examples (beyond that number of digits can happen in modern multi-probe Neuropixels, but I think at that point it looks fine, e.g., ch1024)
| time_reference_channels: str | None = None | ||
| ground: str | None = None | ||
| # recording_mode: str | None = None # TODO: icephys only | ||
| recording_mode: str | None = None |
There was a problem hiding this comment.
IIUC this is only meaninful in icephys, should we keep some comment?
There was a problem hiding this comment.
There are many fields which are subtly used in only one or the other; these are expressed or inferred below in the initializer from NWB contents but not validated outside that
There was a problem hiding this comment.
(So I'd rather not leave comments and do something more formal in a follow-up if that is something you feel strongly about checking)
There will be another round when additional metadata is exposed and I will check all of those kinds of fields then |
Co-authored-by: Austin Macdonald <[email protected]>
Now that the BEP is officially submitted in a somewhat frozen state I can finally go through and update things