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

[ENH] microelectrode electrophysiology specification (BEP032) #1705

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

yarikoptic
Copy link
Collaborator

@yarikoptic yarikoptic commented Feb 14, 2024

Replaces #1352 submitted from a fork outside of bids-specification.

Add specification for microelectrode electrohpysiology datasets based on the BEP032 proposal

Note

We meet regularly and everyone is welcome

Next meeting: insert date on URL to join

Communication channel: https://framalistes.org/sympa/info/neuroscience-data-structure


TODOs

Issues this PR would likely to address

Issues to see being addressed while working on this BEP (likely to move above) or not (moved below):

Other issues which relate but not in scope here and provided for reference/backreference

@@ -42,6 +42,38 @@ age:
for privacy purposes.
type: number
unit: year
alpha_rotation:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really recommend renaming the rotation axes to yaw, roll, and pitch (that would be the analogous angle order). There was no consensus either way on the google docs discussion. Someone said both are confusing, which I guess might be expected, but alpha, beta, gamma, are just more confusing...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree -- very adhoc. Let's discuss in google doc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.google.com/document/d/1oG-C8T-dWPqfVzL2W8HO3elWK8NIh2cOCPssRGv23n0/edit?disco=AAAA4fkI4eY

Could you chime in? I think the other guy commenting might be amenable to accepting this as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that both are confusing, but at least alpha, beta and gamma are SO confusing that everyone realizes that additional specification is needed to define them properly. With roll, yaw and pitch it seems at first that all is clear, until you have a number of different people go through different use cases. See the more challenging examples that I posted on the google doc under https://docs.google.com/document/d/1oG-C8T-dWPqfVzL2W8HO3elWK8NIh2cOCPssRGv23n0/edit?disco=AAAA4fkI4eY

Copy link
Collaborator

@TheChymera TheChymera Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertoostenveld sorry, I only now saw the update, reply there. I don't think confusion on purpose is good in this case, because the documentation is very eclectic so we might be sending people down rabbit holes. Wiki, where people will invariably go first, does a particularly poor job explaining both euler and TB angles for the casual non-mathematics-versed user. The only thing that wiki has going for it here are the aircraft animations on the TB page. Yaw, Pitch, Roll, will be intuited correctly as long as we specify the starting postion. That we can do (1) as (I think, it's pretty vague) is currently proposed, aligning the implant with the world coordinate system (meaning most implants will be at yaw 0 pitch -90 roll 0) or (2) relative to the implantation stereotax normal (meaning most implants will be at 0 0 0).

For comparison, Euler commonly has the normal pointing up so most implants will be.... 0 -180 0 🤔

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the last BEP032 meeting we discussed further and wanted to follow this approach now (based on the Allen Inst. standard and IBL standard):

Assumption: x,y,z is posterior, ventral, right (unit needs to be specified).

Translational origin (0,0,0) needs to be defined (typically Bregma for rodents).

Rotational origin (0,0,0) is the probe facing up with the tip facing forward. Rotations are all clockwise in degrees and around the tip. For multi-shank probes, the “tip” of the probe is defined as the end of the left shank if you are looking at the electrodes.

  • yaw: clockwise when looking down
  • pitch: In the direction of the electrode face
  • roll: clockwise when looking down at the probe

The depth (unit needs to be specified) is a translation in the direction that the tip is pointing.

We need to add a “probe_model”, which references probeinterface_library

  • versioning of probe library files (raised here)
  • annotation of “tip” position (raised here)

NOTE: We need to change the electrode x,y,z.
X,y,z in BIDS refers to location in brain, not on probe.

@@ -391,6 +462,12 @@ reference__ieeg:
- type: string
enum:
- n/a
reference_atlas:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The “reference atlas” if you visually verify the area is basically the atlas you looked at before you nod and said “eh, good enough”. This seems at once an overly detailed (does this really matter? the coordinates are commonly given with sub-mm precision) and underdetermined (what coordinates did you look at in the atlas?). I think it's best just to drop this.

@TheChymera TheChymera closed this Mar 7, 2024
@TheChymera TheChymera deleted the bep032 branch March 7, 2024 22:13
@TheChymera TheChymera restored the bep032 branch March 7, 2024 22:15
@TheChymera TheChymera reopened this Mar 11, 2024
@TheChymera
Copy link
Collaborator

TheChymera commented Mar 12, 2024

@TheChymera
Copy link
Collaborator

@Remi-Gau Remi-Gau changed the title [ENH] Add BEP032 (microelectrode electrophysiology) specification [ENH] microelectrode electrophysiology specification (BEP032) Apr 18, 2024
@Remi-Gau
Copy link
Collaborator

Random suggestion: maybe having a markdown file in the source tree and the mkdoc config so you can at least use a macro to render the filename template.

The HTML version of this could then be reviewed by people still working on the google doc: may help if you don't read BIDS schema yaml fluently.

@Remi-Gau Remi-Gau added the ephys label Apr 19, 2024
@Remi-Gau
Copy link
Collaborator

Did a few minor fix to pacify pre-commit and make sure the HTML page rendered.

Also check the top message of this PR:
#1705 (comment)

I added some github admonitions that need tweaking so people know where this BEP is discussed.

I think this is what might have freaked out `schemacode_ci / windows-latest with Python 3 (pull_request) ` (windows only!) fails with a bunch of

```
2024-04-19T19:13:44.2815607Z self = <encodings.cp1252.IncrementalDecoder object at 0x0000028D6A14D850>
2024-04-19T19:13:44.2818292Z input = b'---\nHED:\n  name: HED\n  display_name: HED Tag\n  description: |\n    Hierarchical Event Descriptor (HED) Tag.\n   ...ed or ideal position along the z axis.\n  anyOf:\n    - type: number\n    - type: string\n      enum:\n        - n/a\n'
2024-04-19T19:13:44.2820386Z final = True
2024-04-19T19:13:44.2820609Z
2024-04-19T19:13:44.2820821Z     def decode(self, input, final=False):
2024-04-19T19:13:44.2821664Z >       return codecs.charmap_decode(input,self.errors,decoding_table)[0]
2024-04-19T19:13:44.2823260Z E       UnicodeDecodeError: 'charmap' codec can't decode byte 0x9d in position 2105: character maps to <undefined>
2024-04-19T19:13:44.2824229Z
2024-04-19T19:13:44.2824768Z C:\hostedtoolcache\windows\Python\3.12.3\x64\Lib\encodings\cp1252.py:23: UnicodeDecodeError
```
@yarikoptic yarikoptic force-pushed the bep032 branch 2 times, most recently from a0068b2 to f03084b Compare April 19, 2024 22:16
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.92%. Comparing base (1cb92eb) to head (73012b3).
Report is 36 commits behind head on master.

Current head 73012b3 differs from pull request most recent head a46e439

Please upload reports for the commit a46e439 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    bids-standard/legacy-validator#1705      +/-   ##
==========================================
+ Coverage   87.79%   87.92%   +0.13%     
==========================================
  Files          16       16              
  Lines        1360     1375      +15     
==========================================
+ Hits         1194     1209      +15     
  Misses        166      166              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…es (#1806)

* RF: to have "microephys" (Microelectrode physiology) for modality and icephys and ecephys for suffixes and datatypes

* Reflecting decision of having two separate datatypes under the Microelectrode Electrophysiology

#1800 (comment)

Consensus reached during working group meeting on 2024-05-15:

- modality = "Microelectrode Electrophysiology"
- datatypes = "icephys" and "ecephys"
- suffixes = "_icephys" and "_ecephys"

* Adjust wording to Horea's recommendation

* Various fixups and tune ups to wording from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants