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.

Sign up for GitHub

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

Jump to bottom

[FIX] What is a composite instance? Change to measurement for non MRI modalities? #813

Merged
merged 10 commits into from
Jun 9, 2021
Merged

[FIX] What is a composite instance? Change to measurement for non MRI modalities? #813

merged 10 commits into from
Jun 9, 2021

Conversation

rob-luke
Copy link
Member

@rob-luke rob-luke commented Jun 1, 2021 •
edited
Loading

What is a composite instance? I had never heard this term before and don't believe it is used in EEG or fNIRS. I googled it and it appears to be a DICOM specific term and thus fMRI specific? But I found the terminology in EEG MEG iEEG and Physiological pages.

I suggest either changing it to measurement (as in this PR), or adding a definition somewhere in the specification.

This is an issue as a PR. Hope that's ok.

@rob-luke rob-luke requested a review from sappelhoff as a code owner June 1, 2021 08:53
sappelhoff
sappelhoff reviewed Jun 1, 2021
View reviewed changes
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

thanks @rob-luke, this looks sensible.

Note that #774 aims to render all these tables from the schema, hence my comment to try and make the texts consistent. cc @tsalo is there something from the schema side to do here?

src/04-modality-specific-files/06-physiological-and-other-continuous-recordings.md Outdated Show resolved Hide resolved
@sappelhoff
Copy link
Member

PS: linkchecker fails due to biosemi.com failing to renew their certificates (?) ... https fails, but http works. You could just change the link from https to http for now. I am not sure when they'll manage to get their https back up.

@rob-luke
Copy link
Member Author

rob-luke commented Jun 1, 2021

FYI @mshader (#802 (comment))

@sappelhoff sappelhoff requested a review from tsalo June 6, 2021 07:49
@rob-luke
Copy link
Member Author

rob-luke commented Jun 8, 2021

@sappelhoff I think I addressed all issues. How/who do I ping for review? There seems to be ways to ping a team, is that what I should do? And which team?

tsalo
tsalo approved these changes Jun 9, 2021
View reviewed changes
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.

The changes LGTM.

@sappelhoff
Copy link
Member

@rob-luke these are the teams you can ping: https://github.com/orgs/bids-standard/teams/everyone --> all nested under "everyone", so if you ping [at]bids-standard/everyone, EVERYBODY will get a notification ... if you go down the nesting, the amount of pings reduces accordingly.

Usually it's fine to ping one of the maintainers that you last were in touch with about an issue/pr.

@rob-luke
Copy link
Member Author

rob-luke commented Jun 9, 2021

Thanks @sappelhoff for explaining. Looks like this is good to go then :)

@sappelhoff
Copy link
Member

Yes, I think so too, will merge! Thanks a lot :-)

Just a note for you: Usually we wait about 5 days after the most recent commit before merging a PR, to give the community enough time to see it, see: https://github.com/bids-standard/bids-specification/blob/master/DECISION-MAKING.md#rules

But in this case it's a relatively uncontroversial fix that has been approved and open for many days now without many changes.

@sappelhoff sappelhoff merged commit d0853ee into bids-standard:master Jun 9, 2021
@rob-luke
Copy link
Member Author

rob-luke commented Jun 9, 2021

Will do @sappelhoff, and Ill be sure to read through that document too.

@rob-luke rob-luke deleted the composite-measurement branch June 9, 2021 07:49
bids-maintenance added a commit that referenced this pull request Jun 9, 2021
@bids-maintenance
[DOC] Auto-generate changelog entry for PR #813
e7e7118
tsalo added a commit that referenced this pull request Jun 9, 2021
@tsalo
composite instances --> measurements
6ebde13 
Changes from #813.
@sappelhoff sappelhoff mentioned this pull request Jul 1, 2021
Merged
27 tasks
tsalo added a commit that referenced this pull request Jul 13, 2021
@tsalo @effigies
@Remi-Gau @sappelhoff @mnoergaard
[SCHEMA] Add metadata term files (#774)
5de7cfc 
* [SCHEMA] Add metadata term files (#762)

* Draft a handful of metadata term files.

* Add example with specific possible values.

* Match the validator schemas better.

* Fix formatting.

* Fix formatting again!

* Draft semi-functional rendering functions.

* Use unit abbreviations.

* Add more fields.

* Get macro working.

* Add terms from first table.

* Add AnatomicalLandmarkCoordinateSystem.

* fMRI task information table.

* More terms.

* More terms.

* More terms.

* EchoTime and FlipAngle

* Add tables.

* More tables.

* More terms.

* More terms.

* More terms.

* Fix spacing.

* Clean things up.

* More terms.

* More terms!

* More terms.

* Some iEEG terms.

* More iEEG terms.

* Add ASL labeling terms.

* Next batch.

* More terms.

* More terms.

* More terms.

* Fix mistakes.

* Last terms.

* Reference yamls.

* Fix mistakes.

* Change format of coordinate system files.

* Use degree in associated files.

* Some of the missing terms.

* A few more terms.

* Fix typos.

* More terms.

* More terms.

* More terms.

* More terms.

* More terms.

* More terms.

* Last terms.

* Fix link.

* Fix internal links.

* Fix links for real.

* Derivative terms.

* Fix up code link.

* Use backslashes for continued strings.

* Replace $ref with file contents.

Also support plural datatype strings and all manner of newlines in descriptions.

* Fix genetics.

* Describe the structure of metadata YAML files.

* Make metadatatype function recursive.

* Improve search function.

* Start adding PET fields.

* Add some fields.

* More terms.

* More terms.

* More terms.

* Fix mistakes.

* More terms.

* Replace InstitutionDepartmentName with existing InstitutionalDepartmentName.

* More terms.

* More terms.

* More terms.

* More terms.

* More terms.

* More terms.

* More terms.

* Last terms.

* Add unit format for strings.

Unused for now but could be useful later.

* Add dataset_relative and participant_relative string formats.

* Update READMEs.

* Fix formats in README.

* Support table-specific metadata description extensions.

* Employ description extensions with IntendedFor.

* Remove explicit defaults from YAML files.

* Replace Minimum with minimum.

* Replace inclusiveMaximum with maximum.

* Replace implicit links with explicit ones.

* Rename key_name to name.

* Rename "Unit" to "Units"

* Improve make_metadata_table docstring.

* Start addressing inconsistencies between rendered and hardcoded tables.

* Fix typos in PET metadata

From #786.

* Add metadata fields from qMRI appendix.

* Fix.

* Address duplicate datatypes.

Should address the "string or string or string or string" issue.

* Wrap example strings in code.

* Use enum for n/a instead of pattern.

It's easier to identify as a special case.

* Replace "string" with "n/a" when appropriate.

* Address some inconsistencies.

* Take a crack as SpatialReference.

The type is complicated, but I _think_ I've got it figured out.

* Apply suggestions from code review

Thanks @effigies!

Co-authored-by: Chris Markiewicz <[email protected]>

* Update tools/schemacode/schema.py

* search_structure --> dereference_yaml

* Use faster loading approach.

* Fix deprecation link.

* Apply suggestions from code review

Co-authored-by: Chris Markiewicz <[email protected]>

* Update 01-magnetic-resonance-imaging-data.md

* Add B0FieldIdentifier and B0FieldSource.

* Revert type changes and add TODOs to check them.

* Update tools/schemacode/schema.py

* Replace remaining relative links.

* Apply suggestions from code review

Co-authored-by: Chris Markiewicz <[email protected]>

* Add new coordinate systems from #775.

* Grab hack from #781 (which wasn't merged).

* Create HED.yaml

* boldify table headers

* add device info metadata

* fix table fences

* fix cell padding

* fix cell padding

this is getting old VERY quickly

* Fix up DICOM tags in metadata.

* Leverage "name" field for section-specific metadata definitions.

* composite instances --> measurements

Changes from #813.

* Fix name of HED field.

* Fix string formatting in coordinate system fields.

* Move "preferably same as" to section-specific text.

For #774 (comment)

* Standardize DICOM Tag format.

Still need to move the references out of the generic definitions.

* Move mentions of DICOM Tags out of definitions.

Only for fields that *also* appear in modalities that *don't* use DICOM.

* Apply suggestions from code review

Co-authored-by: Stefan Appelhoff <[email protected]>

* Generalize SoftwareFilters example.

* Distinguish AnatomicalLandmarkCoordinates definitions.

* Rename fmapEchoTime to match new format.

* Apply suggestions from code review

Co-authored-by: Stefan Appelhoff <[email protected]>

* Apply suggestions from code review

Co-authored-by: Stefan Appelhoff <[email protected]>

* Address review.

* Fix example manufacturer names.

* TEMPORARY: fix osipi URL (revert when osipi.org is back)

* Fix regex for identifying macros.

* Partially address review.

* Do not assume minItems is 1 for array terms.

* composite instances --> measurements (again)

* Update src/schema/metadata/MagneticFieldStrength.yaml

Co-authored-by: Stefan Appelhoff <[email protected]>

* Update description to PharmaceuticalDoseTime

#774 (comment)

* Update 09-positron-emission-tomography.md

* fix table pipe alignment

* add pharmaceuticaldosetime fix to schema

includes a bugfix to convert "should" (not casing, this was not intended
as a SHOULD) to a MUST.

* fix str examples

* Apply suggestions from code review

Co-authored-by: Stefan Appelhoff <[email protected]>

* Update src/schema/metadata/RepetitionTimeExcitation.yaml

Co-authored-by: Stefan Appelhoff <[email protected]>

* Add DoseCalibrationFactor.

* Update ScanDate definition and deprecate it.

* Remove hardcoded tables.

* Remove unused links.

* Update tools/schemacode/schema.py

Co-authored-by: Chris Markiewicz <[email protected]>

Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Remi Gau <[email protected]>
Co-authored-by: Stefan Appelhoff <[email protected]>
Co-authored-by: mnoergaard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@effigies effigies effigies left review comments

@tsalo tsalo tsalo approved these changes

@sappelhoff sappelhoff sappelhoff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rob-luke @sappelhoff @effigies @tsalo