-
Notifications
You must be signed in to change notification settings - Fork 169
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
[ENH] Add explicit wording on DICOM terms correspondence #1450
Conversation
Dear |
I like the proposal, although it almost seems like it should go in contributing docs? |
here is our mapping for PET https://github.com/openneuropet/PET2BIDS/blob/main/metadata/dicom2bids.json |
I think pretty much all of the common principles are pertinent to contributing. I have added an item to TODOs from @effigies that we might want even to list/enforce them at the level of schema. |
Thank you @tsalo for having a look. Looking at https://github.com/bids-standard/bids-specification/blob/master/CONTRIBUTING.md -- they are really oriented toward our setup/technologies and formats used for development of BIDS specification. Common principles on the other hand formulate base principles of BIDS as a standard and composition of its elements. After re-reviewing CONTRIBUTING and common principles -- I think it is a good location ATM. ATM because it relates to the idea/effort of reducing domain specificity in common principles for BIDS 2.0
It somewhat also similar/relating to some of which IMHO should actually be described in bids-specification itself since they are "common principles" behind some choices made within context of BIDS itself after such BEPs are merged. But "bids-extensions" document is not a good place for this PR since, echoing above said, more about "common principles" for what is already in BIDS, or what could be added outside of full fledged BEP mechanism. |
we do use 'Corresponds to DICOM Tags' in the spec so to me it makes sense to have it in common principles as you did -- see for instance https://bids-specification.readthedocs.io/en/stable/modality-specific-files/positron-emission-tomography.html#radiochemistry |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1450 +/- ##
=======================================
Coverage 87.93% 87.93%
=======================================
Files 16 16
Lines 1351 1351
=======================================
Hits 1188 1188
Misses 163 163 ☔ View full report in Codecov by Sentry. |
I don't get how these changes could trigger CI fails which didn't happen within e.g. recent #1758 ... may be I will rebase for a good measure. |
Thank you @CPernet for the feedback. I have pushed another change for "Based on" and consider this PR "ready for review and potentially merge". If agree with it -- give a blessing please ;) I also filed a number of related issues/PRs which reference this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍
Got 2 approvals, was out here for almost a year... any @bids-standard/maintainers who could do the final look and do the honors of clicking "Merge"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tiny wording tuning suggestion. And a general statement that this looks fine to me.
I am not deep enough into the MRI spec to do the final merge, however.
src/common-principles.md
Outdated
Many of such metadata entries are explicitly formalized in BIDS and could be found listed in the [Glossary](./glossary.md). | ||
Whenever formalizing a new metadata entry within BIDS, the name SHOULD correspond to the original DICOM Tag whenever the value is taken *as is* without any further harmonization. | ||
The description of the term in the schema then SHOULD describe such correspondence as "Corresponds to DICOM Tag ID1, ID2 `DICOM Tag Name`.". | ||
If value is harmonized, the description SHOULD describe such correspondence as "Based on DICOM Tag ID1, ID2 `DICOM Tag Name`.". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If value is harmonized, the description SHOULD describe such correspondence as "Based on DICOM Tag ID1, ID2 `DICOM Tag Name`.". | |
If the name is harmonized (against the previous recommendation), then the description SHOULD describe such correspondence as "Based on DICOM Tag ID1, ID2 `DICOM Tag Name`.". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels that it is two points here:
- It might indeed be the name and/or the value which is "harmonized". Previous wording was aiming only for
value
harmonization, @sappelhoff , you propose to flip it toname
here?name
harmonization: For thename
-only (value stays the same) I do not immediately can come up with an example but potentially it could happen.- overall, good point, will think about it a little more.
- Overall wording could be improved to make two cases more obvious (need to think more too, I have some dislike to suggested "against ..." formulation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but potentially it could happen
then let's try to cover both 👍
I was mostly trying to streamline the language, please feel free to iterate!
Co-authored-by: Stefan Appelhoff <[email protected]>
I was pinged a long time ago, and I suppose I should comment. I don't really like instructions to spec writers in the spec. This seems to fit better in a contribution guide. Perhaps a way to rewrite this that would feel more spec-like would be:
This way it indicates to readers what to expect, and indirectly instructs writers how to write these two cases. |
Co-authored-by: Chris Markiewicz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under impression that we had such wording somewhere but I failed to find. Such aspect came up during recent presentation by @rordenlab to SC,
and we already have lots of metadata fields formalized that way
but it seems lacking formalization of that.
Possible TODOs
Imaging files
since DICOMs could be the source of other data types (e.g., physio). What about RFing into a dedicated sectionData converted from DICOM files
or alike preceding specificImaging files
?might be worth establishing correspondence at the level of the schema.DONE File a dedicated issue for this. Add support for relating to DICOM fields directly in the schema #1759