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

[BUG] Inconsistent lengths of indices and rank does not raise an error #158

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

tsbinns
Copy link
Collaborator

@tsbinns tsbinns commented Nov 28, 2023

Problem

A small but notable bug I just came across:

indices and rank should both have shape (2, n_cons), however if the number of connections differs, e.g.

indices = ([[0, 1], [2, 3]],  # 2 seeds
           [[4, 5], [6, 7]])  # 2 targets

rank = ([2],  # 1 seed
        [2])  # 1 target

this does not immediately raise an error. Instead, the use of zip elsewhere in the code to iterate over the entries of indices and rank for each connection means that only the results for the smallest number of connections (in the above example 1 of 2) would be processed.

Example of where zip is used:

for seed_idcs, target_idcs, seed_rank, target_rank in zip(
indices[0], indices[1], ranks[0], ranks[1]):


Proposed fix

Simply adding a check in _check_rank_input that the number of connections in indices and rank match is enough to catch this early on:

if (
len(rank) != 2 or len(rank[0]) != len(indices[0]) or
len(rank[1]) != len(indices[1])
):
raise ValueError('rank argument must have shape (2, n_cons), '
'according to n_cons in the indices')


I have also updated the unit tests to reflect this change.


Sorry for letting this slip through!

@larsoner larsoner merged commit 241b6ff into mne-tools:main Nov 28, 2023
13 checks passed
@larsoner
Copy link
Member

Thanks @tsbinns !

@tsbinns tsbinns deleted the pr-rank_indices_bug_fix branch November 28, 2023 14:48
tsbinns added a commit to tsbinns/mne-connectivity that referenced this pull request Dec 15, 2023
larsoner added a commit to larsoner/mne-connectivity that referenced this pull request Jan 9, 2024
* upstream/main: (56 commits)
  [CI] Fix CIs (mne-tools#164)
  Fix azure
  Fix CIs
  New dev for v0.7
  [RELEASE] V0.6 (mne-tools#162)
  Try azure again
  [CI] Fix azure (mne-tools#161)
  Add gitblame
  [MAINT] Run black, isort, ruff, and other auto-linters on entire package (mne-tools#159)
  [MAINT] Replace setup.py with pyproject.toml (mne-tools#160)
  [BUG] Fixed issue w/ different rank-indices length (mne-tools#158)
  [MAINT] Refactor bivariate and multivariate methods into separate files (mne-tools#156)
  [MAINT] Fix failing unit tests & update CI packages (mne-tools#157)
  fixed grammar mistake
  switched to array indices & added inline comments
  updated default non-zero rank tolerance
  removed redundant list creation
  removed redundant ignored word
  switched to masked indices for multivariate conn
  updated time
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants