Skip to content

Conversation

@ghisvail
Copy link
Contributor

@ghisvail ghisvail commented Sep 22, 2021

Dataset description files are a requirement for BIDS compliance.

  • Define a proper model for the BIDS dataset description
  • Ensure the model is used in bids utility functions
  • Add missing dataset description for some converters

@ghisvail ghisvail force-pushed the bids-dataset-description branch 2 times, most recently from 320f83a to 927a1f7 Compare September 22, 2021 12:57
@ghisvail
Copy link
Contributor Author

Note that the current definition of the BIDS dataset description model is the most nominal one based on the current specs. But it's implemented in a way that future extension only requires adding a properly typed attribute to the class and a field-to-alias mapping in Config.fields if the field is not serialized to pascal case.

@ghisvail ghisvail force-pushed the bids-dataset-description branch from 927a1f7 to 5ee9190 Compare September 22, 2021 13:16
@ghisvail ghisvail requested a review from omar-rifai September 22, 2021 13:28
@ghisvail ghisvail force-pushed the bids-dataset-description branch from af5eed9 to d8ec8c5 Compare September 22, 2021 14:15
@ghisvail ghisvail added this to the 0.5.2 milestone Sep 23, 2021
@ghisvail
Copy link
Contributor Author

ghisvail commented Nov 4, 2021

Blocked by #392, which implements a dataset description model for HABS that could be used for the other converters targeted in this PR. Depends which one we want to merge first between #392 and this one.

@ghisvail ghisvail mentioned this pull request Nov 4, 2021
@ghisvail ghisvail force-pushed the bids-dataset-description branch from d8ec8c5 to a41d085 Compare November 8, 2021 14:12
@ghisvail ghisvail force-pushed the bids-dataset-description branch from a41d085 to c687433 Compare November 8, 2021 14:33
@ghisvail
Copy link
Contributor Author

ghisvail commented Nov 8, 2021

We still need to update the CI data to include the missing dataset_description.json file for some converters. To be done after the current CI refactoring and testing efforts.

@ghisvail
Copy link
Contributor Author

Before we consider merging this, I'd like to test the same approach with attrs (an alternative to pydantic), which is used by pydra extensively. If we were to consider switching to pydra at some point, we might benefit from using the same dependencies.

@ghisvail
Copy link
Contributor Author

I believe the alternative solution in #535 using attrs is better from a conceptual and maintenance perspective. No need to learn the intricacies of pydantic (especially the Config inner class) and closer to the composition over inheritence principle where Python has been going lately. The attrs solution is basically a dataclass (record type) composed with a converter registry provided by cattrs. Simple.

@omar-rifai
Copy link
Contributor

Should we close this PR?

@ghisvail
Copy link
Contributor Author

ghisvail commented Jan 4, 2022

It will be closed automatically once #535 is merged

@ghisvail ghisvail closed this in #535 Jan 5, 2022
@ghisvail ghisvail deleted the bids-dataset-description branch January 5, 2022 15:14
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