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

Have filenameIdentify use datasetContext to determine which filename rules to use. #1939

Merged

Conversation

rwblair
Copy link
Member

@rwblair rwblair commented Apr 19, 2024

Currently is applying derived filename rules when it shouldn't be. This makes it difficult to notify a user that has a derivatives dataset mistakenly listed in dataset_description as a derivative. Something that may have never happened before. I'm fine overlooking that for now.

Should we have deriv datasets not apply any schema.rules.files.raw rules? I think thats why we went through all the trouble of making $refs work for filenames in the schema.

Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.59%. Comparing base (dc61d0f) to head (c5b348e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1939      +/-   ##
==========================================
+ Coverage   85.58%   86.59%   +1.01%     
==========================================
  Files          91      130      +39     
  Lines        3768     5970    +2202     
  Branches     1207     1484     +277     
==========================================
+ Hits         3225     5170    +1945     
- Misses        457      709     +252     
- Partials       86       91       +5     

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

@effigies
Copy link
Collaborator

Should we have deriv datasets not apply any schema.rules.files.raw rules?

Raw files are valid in derivative datasets, so there shouldn't be a need to prune them. They may be redundant with the more lax derivatives rules, however, assuming we're thorough enough in keeping derivative counterparts to all raw rules.

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.

This LGTM. Don't know if your question and my answer impact whether you want to do anything else. Go ahead and merge, if not.

@rwblair
Copy link
Member Author

rwblair commented Apr 22, 2024

Given how marginal the performance gains from pruning the deriv branch were I'll leave it how it is right now.

@rwblair rwblair merged commit 44147eb into bids-standard:master Apr 22, 2024
29 of 32 checks passed
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