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

[FIX] Change recording entity to REQUIRED for pet/blood modality #1005

Merged
merged 3 commits into from
Apr 9, 2022

Conversation

ghisvail
Copy link
Contributor

@ghisvail ghisvail commented Feb 9, 2022

To make it consistent with the rules and the examples

@ghisvail
Copy link
Contributor Author

ghisvail commented Feb 9, 2022

Not sure what the circleci thing is about.

@mnoergaard
Copy link
Collaborator

mnoergaard commented Feb 15, 2022

Hi @ghisvail! Thanks for raising this issue. It states in the PET-BIDS spec that the recording entity is optional, unless several recordings are used (i.e. manual and autosampler). So the example case you are referring to is not incorrect, but the recording entity could in principle be left out, and then the type of recording should be stated in the corresponding json file. I am somewhat hesitant to support your proposed change, because sometimes only the data after combining manual and autosamples is available (+ after additional modeling), and then it would be unclear what entity this data should have, as it is now a mixture between manual and autosamples. In this case, the entity could be left out (because it’s optional) and then described more thoroughly in the json. I know this is actually now derived data after combining the blood recordings, but I have seen cases where only this information is available. I would like not to block the sharing of this data. Let me know what you think of these potential concerns. Also tagging @CPernet and @mathesong to this discussion.

@bendhouseart
Copy link
Collaborator

The intent is for the recording entity to provide clarification if there are multiple blood recording files (or to simply provide more specificity). It looks like inverse is what needs to happen such that the rules yaml needs to be changed from required to optional here.

Would that be satisfactory? Or do you feel there still needs to be an update or additional example added to the examples?

@ghisvail
Copy link
Contributor Author

The lack of consistency between the generated template in the docs, the wording of the paragraph and the example I linked to really makes the interpretation of this part of the spec quite confusing. Assuming the recording entity is indeed optional, then we should go with @bendhouseart suggestion to fix the rules generating the template and the inconsistent example I linked.

@mnoergaard
Copy link
Collaborator

The example you are referring to won't be inconsistent independent of whether the recording entity is required or optional. But after thinking more about this I am inclined to keep the recording entity required, as it provides clarity on the recording type, which is important for the person modelling the blood data and may not be present in the corresponding json file. My previous argument was indeed for derived blood data, so this is considerations to take into account for the PET derivatives.

@mnoergaard
Copy link
Collaborator

Again, thanks for raising this issue @ghisvail.

@ghisvail
Copy link
Contributor Author

You're welcome. Thank you all for taking time reviewing it and providing the additional pieces of context I was missing.

Copy link
Contributor

@melanieganz melanieganz left a comment

Choose a reason for hiding this comment

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

Following the discussion above, I am fine with changing it to be required. The only question for me is what we do if people "only" have the merged (auto plus manual samples) left, e.g. for legacy data. Yes, this would in principle be a derivative, but I wonder if that's not in some cases the only raw data left. Do we anticipate this to become an issue?

@mathesong
Copy link
Contributor

mathesong commented Feb 18, 2022

This feels like the 80/20 rule ought to take precedence here: I think it's going to be a very small minority of PET datasets for which the auto / manual nature of the blood samples is no longer known. I would support the change to make the recording type required.

From a purely pragmatic perspective, I've dealt with data where I've not had access to which samples were auto and manual, and one just has to treat everything as if it's manual data. I think that's quite an uncontroversial take (though of course you could do something fancy with guessing which are which, but it would be need to be tailored for the specifics of the dataset). So one strategy might be to just include in the text that if the auto/manual nature of the data is unknown, that everything can just be labelled as manual?

@ghisvail
Copy link
Contributor Author

So one strategy might be to just include in the text that if the auto/manual nature of the data is unknown, that everything can just be labelled as manual?

I'd be happy to add the proposed change to this PR. Is everyone in favour for it?

@mnoergaard
Copy link
Collaborator

Thanks for your take on this @mathesong - I agree with you. @ghisvail please feel free to add the proposed change to this PR. Thank you.

@melanieganz
Copy link
Contributor

Dear @bendhouseart, we have two approvals now, can you merge please? Thanks!

@ghisvail
Copy link
Contributor Author

Thanks for your take on this @mathesong - I agree with you. @ghisvail please feel free to add the proposed change to this PR. Thank you.

Done 👍

@tsalo tsalo added the consistency Spec is (potentially) inconsistent label Feb 26, 2022
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I think the second edited sentence needs some tweaking. I do agree that this PR should be merged soon though.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

LGTM!

@sappelhoff sappelhoff merged commit 1909cac into bids-standard:master Apr 9, 2022
@sappelhoff
Copy link
Member

Thanks everyone 👍 @mnoergaard could you briefly confirm that the validator does not need to be adjusted due to the changes here?

@mnoergaard
Copy link
Collaborator

LGTM - thanks @sappelhoff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency Spec is (potentially) inconsistent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants