correction of lookup label#84
Conversation
lzehl
left a comment
lzehl
left a comment
There was a problem hiding this comment.
see my comments. TODOs:
- change internal identifier for states to session identifier (when present in bids, or null if not)
- change lookup label for states to
<datasetShortNameCamelCase>_<subjectIdentifier>_<sessionIdentifier>However one could argue for a single conversion/dataset the lookup lable should be<subjectIdentifier>_<sessionIdentifier> - if short name is used change to either given by provider (maybe with max character limit) or default set to short citation.
Hope I did not forget anything
|
I implemented all the requested things, but for short citation, we should do it based on citation.cff, as the information in the dataset_description is not reliable for this at all. |
lzehl
left a comment
lzehl
left a comment
There was a problem hiding this comment.
This does not reflect what we discussed and needs to be revised:
- short name for dataset (version) CANNOT be None. if not provided (I don't like the prompt, I would make this an input and/or a function) needs to be set automatically (e.g. short citation)
- there is no character limit for the short name. we got rid of this a while ago and the old one was more than 10 characters
- lookup label should fall back to one or two solutions as described in https://gitlab.ebrains.eu/ri/projects-and-initiatives/bids2ebrains/bids2ebrains/-/issues/12
| dataset_description = utility.read_json(dataset_description_path.iat[0, 0]) | ||
|
|
||
| if short_name is None: | ||
| while True: |
There was a problem hiding this comment.
I don't understand that part. This should not be a prompt but a function in my opinion and the short name should be set automatically from available information when not specified/overwritten and should not be required for proceeding.
Automatic setting could use the short citation handle (e.g. Zehl et al. 2024) as done in EBRAINS.
|
|
||
|
|
||
| def create_subjects(subject_id, layout_df, layout, collection): | ||
| def create_subjects(subject_id, layout_df, layout, collection, dataset_short_name): |
There was a problem hiding this comment.
short_name or dataset_short_name? what is the difference?
| - ``multiple_files`` (bool, default=False): If True, the OpenMINDS data will be saved into multiple files within the specified output_path. | ||
| - ``include_empty_properties`` (bool, default=False): If True, includes all the openMINDS properties with empty values in the final output. Otherwise includes only properties that have a non `None` value. | ||
| - ``quiet`` (bool, default=False): If True, suppresses warnings and the final report output. Only prints success messages. | ||
| - ``short_name`` (str or bool, default=None): A short name for the dataset (less than 10 characters). If None, you'll be prompted (default); if False, no short name will be assigned. |
There was a problem hiding this comment.
the character limitation is not correct. we don't have this. also if False it cannot just be not defined. It has to be defined. If False a default needs to be set.
| >>> import bids2openminds.converter as converter | ||
| >>> input_path = "/path/to/BIDS/dataset" | ||
| >>> collection = converter.convert(input_path, save_output=True, output_path="/path/to/output", multiple_files=False, include_empty_properties=False, quiet=False) | ||
| >>> collection = converter.convert(input_path, save_output=True, short_name=ShortName, output_path="/path/to/output", multiple_files=False, include_empty_properties=False, quiet=False) |
There was a problem hiding this comment.
is this the convention (CamelCase) how to write string variables?
| break | ||
|
|
||
| # Check length | ||
| if len(input_name) < 10: |
| print( | ||
| "The short name must be fewer than 10 characters. Please try again.\n") | ||
|
|
||
| elif short_name is False: |
There was a problem hiding this comment.
Can't be none later needs to be defined (is a required property and needs to be set as a default)
| }, | ||
| "internalIdentifier": "Studied state sub-01", | ||
| "lookupLabel": "Studied state sub-01" | ||
| "lookupLabel": "sub-01_ses-01" |
There was a problem hiding this comment.
this is not what was discussed prior. As reminder please review issue again for bids2ebrains: https://gitlab.ebrains.eu/ri/projects-and-initiatives/bids2ebrains/bids2ebrains/-/issues/12
| "@type": "https://openminds.ebrains.eu/core/SubjectState", | ||
| "internalIdentifier": "Studied state sub-32", | ||
| "lookupLabel": "Studied state sub-32" | ||
| "lookupLabel": "sub-32_ses-01" |
There was a problem hiding this comment.
same comment as above. please review again https://gitlab.ebrains.eu/ri/projects-and-initiatives/bids2ebrains/bids2ebrains/-/issues/12
3 participants
fixes #83