Fix bad validation check for postion in ElectrodeGroup.init #1770

oruebel · 2023-08-29T10:20:25Z

Motivation

Fix #1768 ElectrodeGroup.position is a compound dataset of x,y,z positions, but ElectrodeGroup.__init__ validates it wrongly by enforcing that the array has 3 elements instead of checking that the elements have 3 elements. This PR updates ElectrodeGroup.__init__ to validate ElectrodeGroup.position correctly

How to test the behavior?

See the linked issue for details

Checklist

Did you update CHANGELOG.md with your changes?
Have you checked our Contributing document?
Have you ensured the PR clearly describes the problem and the solution?
Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
Have you checked to ensure that there aren't other open Pull Requests for the same change?
Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

codecov · 2023-08-29T10:26:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.05%. Comparing base (f136410) to head (c7962f7).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1770      +/-   ##
==========================================
+ Coverage   92.02%   92.05%   +0.03%     
==========================================
  Files          27       27              
  Lines        2620     2630      +10     
  Branches      685      689       +4     
==========================================
+ Hits         2411     2421      +10     
  Misses        139      139              
  Partials       70       70

Flag	Coverage Δ
integration	`73.15% <41.66%> (-0.09%)`	⬇️
unit	`83.95% <91.66%> (+0.02%)`	⬆️

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

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

src/pynwb/ecephys.py

oruebel · 2023-08-29T12:08:28Z

Just some detective work for background:

It seems that ElectrodeGroup.position was added in NWB v2.2 in Support NWB schema 2.2.2 #1146. I believe this bug has existed since the field was added.
Looking at NeurodataWithoutBorders/nwb-schema@59db28e it seems that ElectrodeGroup.position was first added as an array of length 3 (which is what the current check is looking for) and then changed to a compound data type. I.e., my guess is that PyNWB didn't get updated when the change to compound happened, and the unit tests did not fully cover this field.

Given that it has take 3 years for this issue to surface, it seems that ElectrodeGroup.position is probably rarely used.

src/pynwb/ecephys.py

tests/unit/test_ecephys.py

CodyCBakerPhD · 2023-09-14T17:50:15Z

tests/integration/hdf5/test_ecephys.py

@@ -25,7 +25,8 @@ def setUpContainer(self):
        eg = ElectrodeGroup(name='elec1',
                            description='a test ElectrodeGroup',
                            location='a nonexistent place',
-                            device=self.dev1)
+                            device=self.dev1,
+                            position=[(1., 2., 3.), ])


One question - why is this a list at all? Is there a case where we would have multiple tuples in this list? What would that represent?

I also wasn't sure what the use case of the list would be - I added a test case with multiple tuples for now but not sure if that is necessary?

CodyCBakerPhD · 2023-09-14T17:51:34Z

src/pynwb/ecephys.py

+                    position_dtype_valid = False
+            if position_dtype_valid:  # If we have list of element, then check that the elements are of length 3
+                try:
+                    if len(args_to_set['position'][0]) != 3:


Checking only the first element here seems to imply that this value should only be a single tuple of length 2 or 3 and that we would not expect more than one to be passed in the list input

We could iterate through all elements. I believe I just wanted to avoid checking all elements to keep runtime short in case there are a lot of values .

I updated this to iterate and check the length of all the position elements

CodyCBakerPhD · 2023-09-28T17:58:24Z

@oruebel Any updates on this PR?

Co-authored-by: Cody Baker <51133164+CodyCBakerPhD@users.noreply.github.com>

oruebel · 2024-01-25T04:51:12Z

@stephprince could you take a look at this PR. I think this is essentially complete but needs some final attention to get over the finish line.

rly · 2024-03-05T15:35:11Z

@stephprince do you have a chance to take a look at this? thanks!

stephprince · 2024-03-05T16:17:05Z

Yes sorry for the delay I'll take a look!

stephprince · 2024-03-12T15:30:36Z

@rly this is ready to review

rly · 2024-03-13T17:58:00Z

Ok, after doing a full review of the issue and PR, I am also confused as to why ElectrodeGroup.position is allowed to be a list of tuples. I think @oruebel may be mistaken. ElectrodeGroup.position should be a scalar dataset with compound dtype (x, y, z). @oruebel can you review?

oruebel · 2024-05-02T23:52:16Z

I think @oruebel may be mistaken. ElectrodeGroup.position should be a scalar dataset with compound dtype (x, y, z)

The schema here says:

  datasets:
  - name: position
    dtype:
    - name: x
      dtype: float32
      doc: x coordinate
    - name: y
      dtype: float32
      doc: y coordinate
    - name: z
      dtype: float32
      doc: z coordinate
    doc: stereotaxic or common framework coordinates
    quantity: '?'

I.e., position is a compound dataset but the shape is not specified. Looking at the schema language docs here, I believe this means that shape: null is assumed so that it should be scalar. So yes, it seems that this should indeed be a scalar with a compound data type. So this PR should be updated accordingly.

Question, how does the input for scalar compound datasets need to look like? Looking at the source issue #1768 setting:

position=np.array([1., 2., 3.]) : results in an error due to the validation in ElectrodeGroup
position=(1., 2., 3.) : is being written as [(1., 2., 3.) (1., 2., 3.) (1., 2., 3.)]
position=[1., 2., 3.] : is being written as [(1., 1., 1.) (2., 2., 2.) (3., 3., 3.)]

Specifying as [(1, 2, 3), ]) as currently on the PR fixes the bad write. Is position=[(1, 2, 3), ]) the right form and we need to updated the validation to check that only a single tuple is given or is position=(1., 2., 3.) the correct form and we need to dig deeper into write?

oruebel added 2 commits August 29, 2023 03:17

Fix #1768 Bad validation check for postion in ElectrodeGroup.__init__

2beae7c

Update CHANGELOG

e031cd9

oruebel requested a review from rly August 29, 2023 10:22

oruebel added category: bug errors in the code or code behavior priority: low alternative solution already working and/or relevant to only specific user(s) labels Aug 29, 2023

oruebel added this to the Next Release milestone Aug 29, 2023

Updated comments

dc8c1c4

oruebel mentioned this pull request Aug 29, 2023

[Bug]: Writing ElectrodeGroup position saves incorrectly modified data to file #1768

Open

3 tasks

oruebel commented Aug 29, 2023

View reviewed changes

src/pynwb/ecephys.py Outdated Show resolved Hide resolved

Fix spelling in src/pynwb/ecephys.py

60844f2

oruebel requested a review from bendichter August 29, 2023 11:47

CodyCBakerPhD reviewed Sep 14, 2023

View reviewed changes

src/pynwb/ecephys.py Outdated Show resolved Hide resolved

CodyCBakerPhD reviewed Sep 14, 2023

View reviewed changes

src/pynwb/ecephys.py Show resolved Hide resolved

CodyCBakerPhD reviewed Sep 14, 2023

View reviewed changes

tests/unit/test_ecephys.py Show resolved Hide resolved

CodyCBakerPhD reviewed Sep 14, 2023

View reviewed changes

CodyCBakerPhD and others added 3 commits September 28, 2023 13:58

Merge branch 'dev' into fix/electrodegroup_position

826c1d4

Update src/pynwb/ecephys.py

a214ad6

Co-authored-by: Cody Baker <51133164+CodyCBakerPhD@users.noreply.github.com>

Merge branch 'dev' into fix/electrodegroup_position

5272bc3

rly assigned stephprince Mar 5, 2024

stephprince added 4 commits March 5, 2024 08:35

Merge branch 'dev' into fix/electrodegroup_position

169b53d

add len check for all position elements

f4ff605

update CHANGELOG.md

ecd7de2

update docstring to fix broken reference

a21f672

rly and others added 2 commits March 22, 2024 17:01

Merge branch 'dev' into fix/electrodegroup_position

067a618

Merge branch 'dev' into fix/electrodegroup_position

c7962f7

stephprince modified the milestones: 2.7.0, 2.8.0 May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bad validation check for postion in ElectrodeGroup.init #1770

Fix bad validation check for postion in ElectrodeGroup.init #1770

oruebel commented Aug 29, 2023 •

edited

codecov bot commented Aug 29, 2023 •

edited

oruebel commented Aug 29, 2023

CodyCBakerPhD Sep 14, 2023

stephprince Mar 5, 2024

CodyCBakerPhD Sep 14, 2023

oruebel Jan 25, 2024

stephprince Mar 5, 2024

CodyCBakerPhD commented Sep 28, 2023

oruebel commented Jan 25, 2024

rly commented Mar 5, 2024

stephprince commented Mar 5, 2024

stephprince commented Mar 12, 2024

rly commented Mar 13, 2024

oruebel commented May 2, 2024

Fix bad validation check for postion in ElectrodeGroup.__init__ #1770

Are you sure you want to change the base?

Fix bad validation check for postion in ElectrodeGroup.__init__ #1770

Conversation

oruebel commented Aug 29, 2023 • edited

Motivation

How to test the behavior?

Checklist

codecov bot commented Aug 29, 2023 • edited

Codecov Report

oruebel commented Aug 29, 2023

CodyCBakerPhD Sep 14, 2023

Choose a reason for hiding this comment

stephprince Mar 5, 2024

Choose a reason for hiding this comment

CodyCBakerPhD Sep 14, 2023

Choose a reason for hiding this comment

oruebel Jan 25, 2024

Choose a reason for hiding this comment

stephprince Mar 5, 2024

Choose a reason for hiding this comment

CodyCBakerPhD commented Sep 28, 2023

oruebel commented Jan 25, 2024

rly commented Mar 5, 2024

stephprince commented Mar 5, 2024

stephprince commented Mar 12, 2024

rly commented Mar 13, 2024

oruebel commented May 2, 2024

Fix bad validation check for postion in ElectrodeGroup.init #1770

Fix bad validation check for postion in ElectrodeGroup.init #1770

oruebel commented Aug 29, 2023 •

edited

codecov bot commented Aug 29, 2023 •

edited