brainlife / ezbids

A web service for semi-automated conversion of raw imaging data to BIDS
https://brainlife.io/ezbids
MIT License
26 stars 13 forks source link

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

Closed bhatiadheeraj closed 11 months ago

anibalsolon commented 11 months ago

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

dlevitas commented 11 months ago

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

anibalsolon commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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.

dlevitas commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

Ready for review !

dlevitas commented 11 months ago

@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 commented 11 months ago

image image

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

dlevitas commented 11 months ago

LGTM, I'll merge this PR