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

[ENH] implement bids-schema - part 1 #124

Merged
merged 54 commits into from
Feb 15, 2021

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Feb 4, 2021

Use the bids-schema from the specification repo to parse datasets.

  • create function to load schema
  • implement file parsing according to datatypes and modalities of the schema
  • add functions to handle (for a given datatype):
    • entities
    • suffixes
    • extensions
  • implement schema for parsing
  • update field and variable names to fit with the bids terminology (ex: "suffix" instead of "type")
  • support schema less parsing and minimal support of derivatives
  • allow query to return file dependency information

Issues

  • Some suffixes are deprecated in BIDS 1.5 and not included in the schema (e.g FLASH), and therefore will be ignored by layout when parsing them.

  • Different types of "suffixes" allow for different set of entities for the same datatype (see below T1w do have a part entity and T1map does not, even though they are both anat). Relying on the schema when parsing leads to create concatenating structures that have different set of fields.

    • FIXED but creates structure that combine entities from different suffixes that are not ordered.
  • The schema has to be loaded several times when parsing a dataset: this can seriously slow things down (especially on Octave and especially when running the tests where all bids-examples have to be parsed.

    • FIXED though things are still very slow if a lot of datasets have to be parsed.
  {
    "suffixes": [
      "T1w",
    ],
    "entities": {
      "subject": "required",
      "session": "optional",
      "run": "optional",
      "acquisition": "optional",
      "ceagent": "optional",
      "reconstruction": "optional",
      "part": "optional"
    }
  },
  {
    "suffixes": [
      "T1map",
    ],
    "entities": {
      "subject": "required",
      "session": "optional",
      "run": "optional",
      "acquisition": "optional",
      "ceagent": "optional",
      "reconstruction": "optional"
    }
  },

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Feb 4, 2021

UPDATE: NOW FIXED.


CI fails for several errors of the same type.

Nothing similar locally with Octave 4.2, 5.1 or Matlab

failure: cell cannot be indexed with .
  return_datatype_suffixes:5 (/github/workspace/+bids/+internal/return_datatype_suffixes.m)
  test_return_datatype_suffixes>test_return_datatype_suffixes_basic:13 (/github/workspace/tests/test_return_datatype_suffixes.m)
function suffixes = return_datatype_suffixes(datatype)

  suffixes = '_(';

  for iExt = 1:numel(datatype.suffixes)   % < --------------------------------- problem is here (???)
    suffixes = [suffixes,  datatype.suffixes{iExt}, '|']; %#ok<AGROW>
  end

  % Replace final "|" by a "){1}"
  suffixes(end:end + 3) = '){1}';

end

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Feb 4, 2021

OK I give up...

... for now!

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #124 (e318a43) into dev (b0d9bda) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev    #124   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         11      22   +11     
  Lines        864     881   +17     
=====================================
- Misses       864     881   +17     
Flag Coverage Δ
unittests 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
+bids/+internal/add_missing_field.m 0.00% <0.00%> (ø)
+bids/+internal/append_to_layout.m 0.00% <0.00%> (ø)
+bids/+internal/file_utils.m 0.00% <ø> (ø)
+bids/+internal/get_metadata.m 0.00% <0.00%> (ø)
+bids/+internal/keep_file_for_query.m 0.00% <0.00%> (ø)
+bids/+internal/match_structure_fields.m 0.00% <0.00%> (ø)
+bids/+internal/parse_filename.m 0.00% <0.00%> (ø)
+bids/+internal/return_modality_extensions.m 0.00% <0.00%> (ø)
...ids/+internal/return_modality_regular_expression.m 0.00% <0.00%> (ø)
+bids/+internal/return_modality_suffixes.m 0.00% <0.00%> (ø)
... and 17 more

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 3c1ac7b...e318a43. Read the comment docs.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Feb 8, 2021

Fixed problem with CI.

Had to convert cells to structures in several places just for CI.
Not sure of the source of the problem for the difference in behavior though, though I suspect the way JSONs are read in CI gives the JSON schema a different structure.

@Remi-Gau
Copy link
Collaborator Author

Will merge this now before this PR grows even more out of control.
But will first make a list of the features that should be added after this.

@Remi-Gau
Copy link
Collaborator Author

  • allow query to return info about the extension (is table? is zipped?)

@Remi-Gau
Copy link
Collaborator Author

  • schema-less layout also indexes also json files

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Feb 15, 2021

  • generalize usage of intended_for field where applicable and dependency sub-structure

@Remi-Gau
Copy link
Collaborator Author

  • query should return events file metadata and content

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
1 participant