ENH: enable partial metadata indexing #560

jdkent · 2019-12-27T05:09:55Z

closes #558

adds the ability to do partial metadata indexing (to help speed up creating a layout).

codecov · 2019-12-27T05:38:36Z

Codecov Report

Merging #560 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage   82.97%   83.08%   +0.11%     
==========================================
  Files          23       23              
  Lines        2966     2980      +14     
  Branches      749      753       +4     
==========================================
+ Hits         2461     2476      +15     
  Misses        323      323              
+ Partials      182      181       -1

Flag	Coverage Δ
#unittests	`83.08% <100%> (+0.11%)`	⬆️

Impacted Files	Coverage Δ
bids/layout/index.py	`86.72% <100%> (+0.42%)`	⬆️
bids/variables/entities.py	`88.29% <0%> (ø)`	⬆️
bids/layout/layout.py	`85.03% <0%> (+0.17%)`	⬆️
bids/variables/kollekshuns.py	`84.24% <0%> (+0.21%)`	⬆️
bids/variables/variables.py	`88.13% <0%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35e1296...8e46903. Read the comment docs.

adelavega · 2020-01-07T14:56:43Z

bids/layout/layout.py

@@ -184,10 +184,13 @@ class BIDSLayout(object):
        in the root argument is reindexed. If False, indexing will be
        skipped and the existing database file will be used. Ignored if
        database_path is not provided.
-    index_metadata : bool
+    index_metadata : bool or dict


I guess I would rather keep this simple, and if users want to do partial indexing, make them set this to False, and then use the index_metadata function later.

The docstring for the layout is already getting pretty long, and I think this is not a super common use case.

As an aside, maybe in the docs there should be a section on things you can do to improve BIDSLayout performance, and using partial indexing would be one of them.

adelavega · 2020-01-07T15:04:19Z

bids/layout/index.py

+                filters['regex_search'] = True
+            # ensure extension argument is a list
+            if isinstance(filters.get(ext_key), str):
+                filters[ext_key] = [filters[ext_key]]


You can use the listify function from bids.utils without the need for the if clause

Suggested change

filters[ext_key] = [filters[ext_key]]

filters[ext_key] = listify(filters[ext_key])

nice, thank you for the suggestion.

adelavega · 2020-01-07T15:08:54Z

bids/layout/index.py

+            else:
+                ext_key = 'extension'
+                msg = (
+                    "You should explicitly set the extension argument. "


Why does the extension key need to be set to that regex? It seems in BIDSLayout.__init__ setting it to None works

From my use of pybids setting a key to None means do not match any files that contain this key, so my interpretation of extension=None would be to match files without an extension. When I set extensions to None and ran the tests, when this test would fail since none of the nifti files were indexed.

Ah, yeah I guess there's a difference between setting a key to None versus not setting a key at all. So what happens if you don't set it at all?

Just saying because if filters is None, then kwargs does not have extension as a key, yet all meta-data is indexed, correct?

That's a good idea, looks like it indexes the correct information, allowing me to remove the weird logic around what should be specified in the extension key.

That would be nice because then we could cut this whole if clause out.

adelavega · 2020-01-07T15:21:45Z

bids/layout/index.py

        """Index metadata for all files in the BIDS dataset. """
+        if filters:
+            default_ext = ['[.]+']


Suggested change

default_ext = ['[.]+']

Minor: put this close to where it's used

adelavega · 2020-01-07T15:34:24Z

bids/layout/index.py

+            if filters.get('extension'):
+                ext_key = 'extension'
+            elif filters.get('extensions'):
+                ext_key = 'extensions'
+            else:
+                ext_key = 'extension'


Following suggestion replaces this whole section

adelavega · 2020-01-07T15:35:21Z

bids/layout/index.py

+            # ensure we are returning objects
+            filters['return_type'] = 'object'
+            # until 0.11.0, user can specify extension or extensions
+            if filters.get('extension'):


Simplify this a bit. I was going to suggest just renaming extensions to extension if you found it to simplify it further, but then you wouldn't get the DeprecationWarning that .get issues.

Suggested change

if filters.get('extension'):

ext_key = 'extensions' if 'extensions' in filters else 'extension'

if not filters.get(ext_key):

default_ext = ['[.]+']

thank you, this is much cleaner

bids/layout/index.py

Co-Authored-By: Alejandro de la Vega <[email protected]>

tyarkoni

Looks good, but see minor comments.

tyarkoni · 2020-01-08T23:08:49Z

bids/layout/index.py

@@ -164,10 +165,21 @@ def index_files(self):
        """Index all files in the BIDS dataset. """
        self._index_dir(self.root, self.config)

-    def index_metadata(self):
+    def index_metadata(self, **filters):
        """Index metadata for all files in the BIDS dataset. """


Docstring should mention the filters argument explicitly.

thanks for pointing this out, added one in.

tyarkoni · 2020-01-08T23:10:30Z

bids/layout/index.py

+            # ensure we are returning objects
+            filters['return_type'] = 'object'
+            # until 0.11.0, user can specify extension or extensions
+            ext_key = 'extensions' if 'extensions' in filters else 'extension'


.get() will handle the extensions/extension thing for you, so you don't need to worry about it here.

Yeah, he only does because he has to add stuff to the extensions argument, and its not clear which one the user will pass.

…bids into partial_metadata_index

bids/layout/index.py

tyarkoni · 2020-01-09T05:15:06Z

Thanks!

adelavega · 2020-06-06T22:41:39Z

Hey @jdkent is this the correct usage:

bids_layout = BIDSLayout(bids_dir, validate=False, index_metadata=False)

indexer = BIDSLayoutIndexer(bids_layout)
indexer.index_metadata(extension='nii.gz', datatype='func')

I didn't realize when reviewing this PR that index_metadata was not a method of BIDSLayout0, when I made the comment to keep the top level API simple.

Might make sense to add that actually because this is pretty unintuitive to use (confused myself!!)

jdkent · 2020-06-06T23:11:32Z

Yeah, that's how I would use it, (similar usage here)

   # reading in derivatives and bids inputs as queryable database like objects
    layout = BIDSLayout(bids_dir,
                        derivatives=derivatives_pipeline_dir,
                        index_metadata=False,
                        database_file=database_path,
                        reset_database=reset_database)

    # only index bold file metadata
    if reset_database:
        indexer = BIDSLayoutIndexerPatch(layout)
        metadata_filter = {
            'extension': ['nii', 'nii.gz', 'json'],
            'suffix': 'bold',
        }
        indexer.index_metadata(**metadata_filter)

jdkent added 2 commits December 26, 2019 23:05

add ability to partially index metadata

034bcc0

test ability to partially index metadata

f4898c0

jdkent changed the title ~~ENH: partial metadata index~~ ENH: partial metadata indexing Dec 27, 2019

jdkent changed the title ~~ENH: partial metadata indexing~~ ENH: enable partial metadata indexing Dec 27, 2019

adelavega requested changes Jan 7, 2020

View reviewed changes

jdkent added 4 commits January 8, 2020 13:05

revert layout.py to simplify index_metadata

b129ba6

apply @adelavega's suggestions

17a5132

modify index_metadata interface to conform to api

a658bf0

ignore extension keyword if not specified

dfc688d

adelavega reviewed Jan 8, 2020

View reviewed changes

bids/layout/index.py Outdated Show resolved Hide resolved

simplify .get usage

867f4c6

Co-Authored-By: Alejandro de la Vega <[email protected]>

adelavega approved these changes Jan 8, 2020

View reviewed changes

tyarkoni reviewed Jan 8, 2020

View reviewed changes

jdkent added 2 commits January 8, 2020 18:08

add doc string for **filters

56f4047

Merge branch 'partial_metadata_index' of https://github.com/jdkent/py…

e9dc269

…bids into partial_metadata_index

jdkent commented Jan 9, 2020

View reviewed changes

bids/layout/index.py Outdated Show resolved Hide resolved

remove warnings import

8e46903

tyarkoni merged commit d115964 into bids-standard:master Jan 9, 2020

adelavega mentioned this pull request Jul 25, 2024

FIX: Filtering in BIDSLayoutIndexer #1063

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: enable partial metadata indexing #560

ENH: enable partial metadata indexing #560

jdkent commented Dec 27, 2019

codecov bot commented Dec 27, 2019 •

edited

Loading

adelavega Jan 7, 2020

adelavega Jan 7, 2020 •

edited

Loading

jdkent Jan 8, 2020

adelavega Jan 7, 2020

jdkent Jan 8, 2020

adelavega Jan 8, 2020

adelavega Jan 8, 2020 •

edited

Loading

jdkent Jan 8, 2020

adelavega Jan 8, 2020

adelavega Jan 7, 2020

adelavega Jan 7, 2020 •

edited

Loading

adelavega Jan 7, 2020

jdkent Jan 8, 2020

tyarkoni left a comment

tyarkoni Jan 8, 2020

jdkent Jan 9, 2020

tyarkoni Jan 8, 2020

adelavega Jan 8, 2020

tyarkoni commented Jan 9, 2020

adelavega commented Jun 6, 2020 •

edited

Loading

jdkent commented Jun 6, 2020 •

edited

Loading

	filters[ext_key] = [filters[ext_key]]
	filters[ext_key] = listify(filters[ext_key])

ENH: enable partial metadata indexing #560

ENH: enable partial metadata indexing #560

Conversation

jdkent commented Dec 27, 2019

codecov bot commented Dec 27, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

adelavega Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelavega Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelavega Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyarkoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyarkoni commented Jan 9, 2020

adelavega commented Jun 6, 2020 • edited Loading

jdkent commented Jun 6, 2020 • edited Loading

codecov bot commented Dec 27, 2019 •

edited

Loading

adelavega Jan 7, 2020 •

edited

Loading

adelavega Jan 8, 2020 •

edited

Loading

adelavega Jan 7, 2020 •

edited

Loading

adelavega commented Jun 6, 2020 •

edited

Loading

jdkent commented Jun 6, 2020 •

edited

Loading