FIXES: invalid filters, mark all xfails, etc.. #40
Conversation
| # but it was not. Session does not exist in dataset, yet since Query | ||
| # Enum is set to optional the query passes even with 'ignore_filters' |
There was a problem hiding this comment.
Session is a legitimate entity, so we should be able to search for files where session may be any value or absent. The idea is to be able to write queries that will work regardless of dataset. If we can't query for legit entities that are absent, that's a problem.
There was a problem hiding this comment.
Right, but shouldn't that behavior depend on ignore_filters? Seems kinda weird to me to say ignore_filters=error and then not error on session.
I guess we could say all core entities are valid queries
There was a problem hiding this comment.
Well an invalid filter is one that doesn't match any files. session=OPTIONAL matches all files.
There was a problem hiding this comment.
Allright, fair enough, I don't feel strongly -- but is this in the actual BIDS spec or just a pyBIDS thing?
There was a problem hiding this comment.
It is just a pybids thing, but we definitely use it when writing queries. It's useful to say "present but any" (REQUIRED), "absent" (NONE) or "either" (OPTIONAL), and not have to put in a bunch of dataset-dependent logic before constructing your query, or implement your own OPTIONAL and remove it from kwargs.
There was a problem hiding this comment.
And it's not sufficient to just say ignore_filters='ignore' for those types of queries? Is that too "unsafe"?
There was a problem hiding this comment.
How would you implement it with invalid_filters='ignore'? Wouldn't that silently return no files?
There was a problem hiding this comment.
The way we implement it right now is that we just remove OPTIONAL filters from the list. We could check whether the entity is valid and do the same. I think it would also be reasonable for session=NONE to return True for all files in a dataset without sessions.
There was a problem hiding this comment.
Yes, it would, but isn't that obvious when writing the query?
I would be in favor of not throwing errors for all BIDS valid core entities by default. Either way, I think this is all open for revision in the new API
| @@ -479,22 +493,24 @@ def test_get_with_regex_search_bad_dtype(layout_7t_trt): | |||
| # Two runs (1 per session) for each of subjects '10' and '01' | |||
| assert len(results) == 4 | |||
|
|
|||
|
|
|||
| # AD: Changed this test to use a entity found in ds005 | |||
| # 'session' was not in dataset, therefore not suggested | |||
There was a problem hiding this comment.
Isn't the point that session=True is invalid?
There was a problem hiding this comment.
Maybe, depends on how the above is resolved, but I thought it was just a simple test of using the wrong name for an entity (i.e. "ses" instead of "session")
There was a problem hiding this comment.
Oh, that was probably it.
Co-authored-by: Chris Markiewicz <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## rf/ancp-layout #40 +/- ##
==================================================
- Coverage 53.33% 46.35% -6.99%
==================================================
Files 34 34
Lines 3549 3596 +47
Branches 857 876 +19
==================================================
- Hits 1893 1667 -226
- Misses 1489 1787 +298
+ Partials 167 142 -25
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Various fixes:
BIDSFileand respectabsolute_pathsBIDSLayoutparameterQueryENUMI looked at all the tests and here's the summary:
Only 2 "unexpected" failures since they are ancpbids bugs:
the rest are xfails:
derivatives features not implemented:
BIDSLayoutwon't be implemented / not critical:
So all in all, I think the main work is in supporting derivatives, and fixing the two issues w/ ancpbids and I think we'll be 80% of the way there