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

Adding ability do handle MRI metadata to be saved into the BIDS Sidecar #99

Merged
merged 21 commits into from
Nov 2, 2023

Conversation

bhatiadheeraj
Copy link
Collaborator

No description provided.

@anibalsolon
Copy link
Member

@bhatiadheeraj is this still up to review? let us know when it is ready

@dlevitas
Copy link
Collaborator

@anibalsolon I've alerted Dheeraj to a few lingering issues while reviewing, so we can hold off for now.

@anibalsolon
Copy link
Member

@bhatiadheeraj @dlevitas I've removed ourselves as reviewers- to be re-added when the PR is good to review. Please let's try to keep reviews through the issue so others can also understand what's happening.

@dlevitas
Copy link
Collaborator

dlevitas commented Oct 30, 2023

@bhatiadheeraj, this PR is coming along nicely, there's one lingering issue that I failed to mention during our last meeting.

One of the sequences is perf/m0scan, but when clicking on the "Edit Metadata" button, the required fields are as if it's a perf/asl sequence. This is probably due to the fact that the bids-specification asl.yaml file also includes information for not just the asl suffix, but also m0scan, see here.

@dlevitas
Copy link
Collaborator

@bhatiadheeraj disregard my previous message. My recent commit addresses the issue I described.

@anibalsolon feel free to review this PR when you get a chance. I ran through from a user (front-end) perspective, and was able to successfully create and download a BIDS-compliant ASL dataset. I didn't give much consideration to back-end processes, so feel to focus your review on that if you need that's appropriate.

@francopestilli francopestilli changed the title now supports multi type for different fields Adding ability do handle MRI metadata to be saved into the BIDS Sidecar Oct 31, 2023
@dlevitas
Copy link
Collaborator

Something for us to consider, but shouldn't prevent this PR from being merged:

Some metadata fields are optional, but become recommended (or required), depending on the value of other metadata fields. When optional metadata fields become required, they are outlined in red, which alerts the users to this change. If however, optional metadata fields become recommended, the user will be unaware. Additionally, the newly recommended metadata field won't move to the Recommended column. We should eventually address this, but it's not high priority for now, given that this won't trigger a BIDS validation error, nor does it prevent users from entering the information.

@francopestilli
Copy link
Contributor

@bhatiadheeraj can you please add a box around each column in the interface. I mean the following. Currently the interface for the metadata has 3 columns: Required, Recommended and Optional. I think it would be helpful to have each column be marked by a very lightly weighted box. Similar to the ezBIDS landing page box see here.
image

@bhatiadheeraj
Copy link
Collaborator Author

Ready for review !

Copy link
Member

@anibalsolon anibalsolon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -120,7 +141,10 @@ MRIASLPcaslSpecific:
fields:
PCASLType:
level: recommended
level_addendum: if `ArterialSpinLabelingType` is `"PCASL"`
level_addendum: recommended if `ArterialSpinLabelingType` is `PCASL`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PCASLType is already recommended, does the addendum change anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The level_addendum specifies conditions where the metadata might become required (or recommended), based on other metadata values

ui/src/components/modalityForm.vue Outdated Show resolved Hide resolved
ui/src/components/modalityForm.vue Outdated Show resolved Hide resolved
ui/src/components/modalityForm.vue Outdated Show resolved Hide resolved
ui/src/components/modalityForm.vue Outdated Show resolved Hide resolved
ui/src/components/modalityForm.vue Outdated Show resolved Hide resolved
@dlevitas
Copy link
Collaborator

dlevitas commented Nov 2, 2023

@bhatiadheeraj there's no validation to differentiate between number and array-type metadata values. For example, with perf/asl data the "PostLabelingDelay" can be either a number or array value, but if the user specifies the type as number and provides an array (or vice versa), this isn't flagged. We should alert users when this occurs, otherwise this generates a bids-validator error later on.

@bhatiadheeraj
Copy link
Collaborator Author

image
image

It should be resolved now ! To see changes please refresh the page.
Thanks for pointing it out !

@dlevitas
Copy link
Collaborator

dlevitas commented Nov 2, 2023

LGTM, I'll merge this PR

@dlevitas dlevitas merged commit d2a6ec9 into master Nov 2, 2023
1 check passed
@dlevitas dlevitas deleted the perf_asl_multi_type branch November 2, 2023 16:20
@dlevitas dlevitas mentioned this pull request Nov 2, 2023
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.

4 participants