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

FIX: Filtering in BIDSLayoutIndexer #1063

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

Conversation

alperkent
Copy link
Contributor

@alperkent alperkent commented May 10, 2024

Resolves (partially) #1062.

Remove if clause on self.filters
@alperkent alperkent reopened this May 22, 2024
Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.74%. Comparing base (20007e5) to head (9b9c74e).

Files Patch % Lines
bids/layout/index.py 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
- Coverage   89.80%   89.74%   -0.06%     
==========================================
  Files          63       63              
  Lines        7177     7179       +2     
  Branches     1374     1375       +1     
==========================================
- Hits         6445     6443       -2     
- Misses        532      535       +3     
- Partials      200      201       +1     

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

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

I'm realizing that the **filters argument only says it applies to filtering files opened during get_metadata(). It was not intended to filter files.

That said, I can't think of a good reason to filter metadata but not files. @adelavega Do you remember the reasoning here?

Comment on lines +253 to 256
# Skip entities that don't match the filter
if self.filters and e.name and e.name in self.filters and m and m not in self.filters[e.name]:
continue
if m is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the equivalent of

Suggested change
# Skip entities that don't match the filter
if self.filters and e.name and e.name in self.filters and m and m not in self.filters[e.name]:
continue
if m is not None:
if m is not None and (e.name not in self.filters or m in listify(self.filters[e.name])):

@adelavega
Copy link
Collaborator

I'm not sure tbh.

I traced this back to; #560

Then Yarkoni moved it around.

Looks like we thought at the time that meta-data indexing was the most time consuming step, and implemented a way to skip meta-data indexing for a subset of files, rather then excluding them from being indexed altogether.

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.

3 participants