-
Notifications
You must be signed in to change notification settings - Fork 45
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
Refactor open ephys #305
base: main
Are you sure you want to change the base?
Refactor open ephys #305
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
==========================================
+ Coverage 89.38% 89.44% +0.05%
==========================================
Files 10 10
Lines 1885 1895 +10
==========================================
+ Hits 1685 1695 +10
Misses 200 200 ☔ View full report in Codecov by Sentry. |
src/probeinterface/io.py
Outdated
if npx_probe[ptype]["shank_number"] > 1: | ||
# Adds a shift to rows in the staggered configuration | ||
is_row_staggered = np.mod(y_pos / y_pitch + 1, 2) == 1 | ||
stagger = probe_stagger if is_row_staggered else 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain to me the case where you would have a probe_stagger but not need it? Or is it possible to have row stagger and column stagger and that is why you are checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can help me to find terminology?
The value on the left stagger
is the displacement for that specific row. Some rows are displaced and some are not and this calculated on the is_row_staggered
variable.
In other words, probe_stagger
is the global variable that characterized the probe whereas stagger
is the one of the row. Maybe it should be called row_stagger
or row_displacement
? Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this was more me misreading. Maybe to make it clearer I would day
is_this_row_staggered = xx
stagger_for_this_row = probe_stager
The this helps emphasize a particular row whereas my confusion was is_row_staggered
made me think you were doing a global check rather than a specific row. But that's a lot of typing. So this is a soft rec now that I see what you mean!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that that is simple change that would improve readability. Pushing it.
for more information, see https://pre-commit.ci
This came up when trying to fix a problem with a missing probe channel in open ephys. While I was trying to understand how the function reads the contact ids I thought that I could do a small improvement on the variable names by extracting some dictionary variables that are accessed many times on the loop.