ElectrodesTable

[mavaylon1]

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

How to test the behavior?

Show how to reproduce the new behavior (can be a bug fix or a new feature)

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]

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (nwb-schema-2.9.0@3de7018). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/pynwb/ecephys.py	92.85%	0 Missing and 1 partial ⚠️
src/pynwb/file.py	90.90%	1 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##             nwb-schema-2.9.0    #1890   +/-   ##
===================================================
  Coverage                    ?   94.77%           
===================================================
  Files                       ?       28           
  Lines                       ?     2795           
  Branches                    ?      715           
===================================================
  Hits                        ?     2649           
  Misses                      ?       87           
  Partials                    ?       59           

Flag	Coverage Δ
integration	74.31% <80.48%> (?)
unit	84.65% <80.48%> (?)

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mavaylon1]

@rly This is at the point where everything works on my end, I added some notes for future reference, and should be good to go. Let me know what you think. Let's see if we can release the schema asap after merging the electrodes schema

[rly]

@mavaylon1 I reconfigured this PR to merge into the nwb-schema-2.9.0 branch where the other schema changes live, so there are no longer errors associated with ExternalImage. I also added back mock_ElectrodeTable for backwards compatibility since that is used elsewhere. I also changed ElectrodesTable.__init__ - fields like "x" and "imp" are optional columns, not optional fields, and should not be passed to __init__ but rather configured in __columns__.

We're testing backwards compatibility with a file tests/back_compat/2.6.0_DynamicTableElectrodes.nwb. This file has schema 2.8.0-alpha. Could you please rename this file to tests/back_compat/2.6.0_DynamicTableElectrodes.nwb and rewrite it using schema 2.8.0 (not alpha)?

[rly]

I asked @stephprince to jump in on this so that we can get this merged tomorrow. One other thing I forgot to mention is that we should include the code used to generate the test files in src/pynwb/testing/make_test_files.py

[mavaylon1]

Okay great thanks for passing this on @rly

[stephprince]

We're testing backwards compatibility with a file tests/back_compat/2.6.0_DynamicTableElectrodes.nwb. This file has schema 2.8.0-alpha. Could you please rename this file to tests/back_compat/2.6.0_DynamicTableElectrodes.nwb and rewrite it using schema 2.8.0 (not alpha)?

@rly I think there was supposed to be a different file name provided here. I renamed to 3.0.0_electrodes_dynamic_table.nwb to mimic the other test files and match the pynwb version used. Unless it should be written with 2.6.0 specifically?

As a side note - I don't think the generate_test_files.yml workflow is currently being triggered by anything. Is it intended to be run somewhere or is it more of a demonstration of how to generate the test files if needed?

[rly]

We're testing backwards compatibility with a file tests/back_compat/2.6.0_DynamicTableElectrodes.nwb. This file has schema 2.8.0-alpha. Could you please rename this file to tests/back_compat/2.6.0_DynamicTableElectrodes.nwb and rewrite it using schema 2.8.0 (not alpha)?

@rly I think there was supposed to be a different file name provided here. I renamed to 3.0.0_electrodes_dynamic_table.nwb to mimic the other test files and match the pynwb version used. Unless it should be written with 2.6.0 specifically?

Yes use version 3.0.0. It should be the last pynwb release before the change.

As a side note - I don't think the generate_test_files.yml workflow is currently being triggered by anything. Is it intended to be run somewhere or is it more of a demonstration of how to generate the test files if needed?

A demonstration / record

Thank you @stephprince !

[rly]

This looks good to me. @mavaylon1 please review the changes that @stephprince and I made to your PR.