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

[ENH] BEP031: Microscopy #881

Merged
merged 76 commits into from
Jan 18, 2022
Merged

[ENH] BEP031: Microscopy #881

merged 76 commits into from
Jan 18, 2022

Conversation

mariehbourget
Copy link
Collaborator

@mariehbourget mariehbourget commented Sep 16, 2021

This PR aims to incorporate the Microscopy BEP031 into the BIDS specification.

It follows PR #812 that integrated the new sample entity and samples.tsv file, and PR #816 still open for addition of animal metadata to the participants.tsv columns.

Link to the rendered draft: https://bids-specification--881.org.readthedocs.build/en/881/04-modality-specific-files/10-microscopy.html

Thank you to everyone who contributed to this BEP!

Moderators: @mariehbourget, @jcohenadad


Note to the maintainers:

  • As discussed, I minimized the number of files for community review by not using yaml files and macro. The only 2 yaml files I edited/added are src/schema/entities.yaml and src/schema/datatypes/microscopy.yaml for the "Entities" and "Entity table" appendix.
  • We have 2 metadata field already existing in BIDS for which our definition is slightly different (StationName and BodyPart). From my understanding of the schema and macros, this is something that we will be able to fix after community review with the yaml files. Let me know if that is correct.
  • There is a "Recommended participant data" section that overlaps with PR [ENH] BEP031 - New columns to participants.tsv file #816 regarding strain and strain_rrid columns in participants.tsv. Its content depends on whether or not those columns will be included in the "general" Participants file section.
    EDIT: The "Recommended participant data" section is now up-to-date following the merge of PR [ENH] BEP031 - New columns to participants.tsv file #816.

Post community-review checklist

  • implement macros
    • for filename templates
    • for examples
    • for metadata table
    • for modality suffix table
  • add contributors to the wiki (so they can be added to the contributors page)
  • update link to bids-examples
  • remove remaining commented sections

Bonus:

@tsalo tsalo added the BEP label Sep 17, 2021
@tsalo
Copy link
Member

tsalo commented Sep 20, 2021

We have 2 metadata field already existing in BIDS for which our definition is slightly different (StationName and BodyPart). From my understanding of the schema and macros, this is something that we will be able to fix after community review with the yaml files. Let me know if that is correct.

It depends on how your definition is different. Do you feel that the altered definition can be incorporated as an addition to the existing one? Is there are more general form of the existing definition that would work better for all applications?

If microscopy requires a completely different definition, then we can support that in the schema, but we try to avoid that situation, so having context-dependent additions to the definition would be preferable.

@mariehbourget
Copy link
Collaborator Author

We have 2 metadata field already existing in BIDS for which our definition is slightly different (StationName and BodyPart). From my understanding of the schema and macros, this is something that we will be able to fix after community review with the yaml files. Let me know if that is correct.

It depends on how your definition is different. Do you feel that the altered definition can be incorporated as an addition to the existing one? Is there are more general form of the existing definition that would work better for all applications?

If microscopy requires a completely different definition, then we can support that in the schema, but we try to avoid that situation, so having context-dependent additions to the definition would be preferable.

In both cases, it's context-dependent replacement, the first sentence (general description) remains the same, but the second sentence (DICOM tag and/or example) is different. So I think the first sentence could remain in the yaml file description, but the second one would go in modality-dependent files. Proposed changes are documented in the comments of 10-microscopy.md on line 213 and 247.

@mariehbourget
Copy link
Collaborator Author

Hi @bids-standard/maintainers ,
This PR is ready for the maintainers review prior to community review (as discussed in our meeting).
I fixed all the markdown formatting issue, however there is still an error from the lint checker that I don’t understand well enough to fix.

We also have opened a PR last week for 2 bids-examples for microscopy (#286)
The validator part is a little behind, but we are working on it and hope to open a PR in the next 2 weeks.

Please let us know if we forgot something or if there is something else we need to do prior to the maintainers review.
Thank you for you help! 😊

@LeeKamentsky
Copy link

LeeKamentsky commented Sep 27, 2021

A minor note on the "ShrinkageFactor" sidecar field: we do tissue expansion - the tissue is osmotically expanded to several times its original size to attain higher resolution from the same microscope objective. The explanation text reads, "Estimated shrinkage factor of the tissue, given in percent (between 0 and 100%)".
The shrinkage is 100 * (original-tissue-size - final-tissue-size) / original-tissue-size. Unfortunately, if the final tissue size is larger than the original, the shrinkage factor is less than zero which is bound to be confusing. For instance, for our current experiment, ShrinkageFactor = -100.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

This looks great, IMO! I have a few comments. Feel free to push back on them...

src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
@mariehbourget
Copy link
Collaborator Author

A minor note on the "ShrinkageFactor" sidecar field: we do tissue expansion - the tissue is osmotically expanded to several times its original size to attain higher resolution from the same microscope objective. The explanation text reads, "Estimated shrinkage factor of the tissue, given in percent (between 0 and 100%)". The shrinkage is 100 * (original-tissue-size - final-tissue-size) / original-tissue-size. Unfortunately, if the final tissue size is larger than the original, the shrinkage factor is less than zero which is bound to be confusing. For instance, for our current experiment, ShrinkageFactor = -100.

Thank you @LeeKamentsky for pointing that out, this is a very interesting point.
We should try to come up with a field that would be able to better describe both “shrink” and “expansion”.

What about a “TissueDeformationScaling” field, expressed as a percentage of the original tissue size, with no upper limit.
For examples:

  • for a shrinkage of 3% --> the value would be 97%
  • for an expansion of 100% --> the value would be 200%.

Let me know your thoughts

@LeeKamentsky
Copy link

Thanks, Marie - your TissueDeformationScaling recommendation would be perfect.

@mariehbourget
Copy link
Collaborator Author

Thanks, Marie - your TissueDeformationScaling recommendation would be perfect.

Thank you Lee, the modification is done in 2a5cf25.

@mariehbourget
Copy link
Collaborator Author

Hi @bids-standard/maintainers,
I addressed all the comments in the PR, the branch is up-to-date with master without conflict, and all of our examples are up-to-date as well.
Would we be ready to open the community review?
Thanks!

@mariehbourget
Copy link
Collaborator Author

Hi @bids-standard/maintainers,
We would like to open the PR to community review. How should we proceed?
Thank you in advance for your help!

@sappelhoff
Copy link
Member

Hey @mariehbourget sorry for not getting back to you sooner - we just discussed this in our biweekly maintainers meeting, and will finish a final, quick pass very soon and then get back to you.

@mariehbourget
Copy link
Collaborator Author

mariehbourget commented Jan 12, 2022

Hi @satra,

At this stage, we have completed the specification, examples and validators for the current supported microscopy formats (PNG, TIF, OME-TIFF).

Making a “clean” addition of the NGFF format would most likely require adding at last one example dataset and do a major update to the validator. Apart from the file extension, the validator also check consistency for metadata between OME-TIFF files and BIDS JSON metadata. I don’t have a good enough knowledge of NGFF at this point to know if and how the same checks should be performed for NGFF files.

I would suggest merging this PR and releasing the microscopy specification as is and treat the NGFF format as an extension to this BEP.

@satra
Copy link
Collaborator

satra commented Jan 13, 2022

@mariehbourget - i can send a separate PR for ngff after this is merged.

@mariehbourget
Copy link
Collaborator Author

Hi @bids-standard/maintainers,

Following the merge of the microscopy examples PR bids-standard/bids-examples#286, I updated the link to the examples in the specs, so the post community-review checklist is complete.

I also added "samples" to src/schema/objects/top_level_files.yaml as I realized it was missing.

All the other comments have been addressed, but there are some conflicts with master related to the schema that I am not comfortable resolving.

@effigies
Copy link
Collaborator

@mariehbourget @tsalo I resolved the conflict. Can you verify that the schema portions of the PR look correct?

@tsalo
Copy link
Member

tsalo commented Jan 13, 2022

@effigies I don't see any issues with the merge. Thanks!

@mariehbourget
Copy link
Collaborator Author

mariehbourget commented Jan 13, 2022

Thanks @effigies!

The merge looks good for the microscopy parts (on the PDF) but the RTD build failed with this error:
�[33mWARNING - �[0mDocumentation file '99-appendices/14-glossary.md' contains a link to '99-appendices/06-longitudinal-and-multi-site-studies.md' which is not found in the documentation files.

EDIT: oops, it seems to be my fault with a broken link here.
I'm unable to fix it because I can't run mkdocs serve locally after the merge. I re-installed the requirements but get these errors from schemacode:

ValueError: Schema path or paths do not exist:
                    /home/mhbourget/venv-bids-2/lib/python3.8/site-packages/schemacode/data/schema/objects
                    /home/mhbourget/venv-bids-2/lib/python3.8/site-packages/schemacode/data/schema/rules

@Remi-Gau Remi-Gau linked an issue Jan 14, 2022 that may be closed by this pull request
@TheChymera
Copy link
Collaborator

I wonder whether it wouldn't be more consistent with BIDS practices to have the suffixes as lower-case, after all cbv and bold are also suffixes which stand for initials, and are lower-case.
The only suffix for which I see some obvious value to uppercase is uCT, where I assume u stands for µ and is less obvious in lowercase.
I don't think this is really enough value to justify breaking with the BOLD/CBV precedent.

@Remi-Gau
Copy link
Collaborator

I wonder whether it wouldn't be more consistent with BIDS practices to have the suffixes as lower-case, after all cbv and bold are also suffixes which stand for initials, and are lower-case. The only suffix for which I see some obvious value to uppercase is uCT, where I assume u stands for µ and is less obvious in lowercase. I don't think this is really enough value to justify breaking with the BOLD/CBV precedent.

hum... I am pretty sure that BEP001 has introduced many precedents for using upper case suffixes:

https://github.com/bids-standard/bids-specification/blob/master/src/schema/objects/suffixes.yaml

So I don't think this should be an issue for concern.

@TheChymera
Copy link
Collaborator

TheChymera commented Jan 16, 2022

@Remi-Gau

I was thinking of suffixes, but you're right, there are even upper-case suffixes:

chymera@decohost ~/src/bids-specification/src/schema $ git rev-parse HEAD
febae4fb9b2c9f055dc1392e0283c85f05618de0
chymera@decohost ~/src/bids-specification/src/schema $ grep -R suffixes rules/datatypes/ -A 1
rules/datatypes/pet.yaml:- suffixes:
rules/datatypes/pet.yaml-  - pet
--
rules/datatypes/pet.yaml:- suffixes:
rules/datatypes/pet.yaml-  - blood
--
rules/datatypes/pet.yaml:- suffixes:
rules/datatypes/pet.yaml-  - events
--
rules/datatypes/ieeg.yaml:- suffixes:
rules/datatypes/ieeg.yaml-  - ieeg
--
rules/datatypes/ieeg.yaml:- suffixes:
rules/datatypes/ieeg.yaml-  - channels
--
rules/datatypes/ieeg.yaml:- suffixes:
rules/datatypes/ieeg.yaml-  - coordsystem
--
rules/datatypes/ieeg.yaml:- suffixes:
rules/datatypes/ieeg.yaml-  - electrodes
--
rules/datatypes/ieeg.yaml:- suffixes:
rules/datatypes/ieeg.yaml-  - events
--
rules/datatypes/ieeg.yaml:- suffixes:
rules/datatypes/ieeg.yaml-  - photo
--
rules/datatypes/beh.yaml:- suffixes:
rules/datatypes/beh.yaml-  - stim
--
rules/datatypes/beh.yaml:- suffixes:
rules/datatypes/beh.yaml-  - events
--
rules/datatypes/perf.yaml:- suffixes:
rules/datatypes/perf.yaml-  - asl
--
rules/datatypes/perf.yaml:- suffixes:
rules/datatypes/perf.yaml-  - aslcontext
--
rules/datatypes/perf.yaml:- suffixes:
rules/datatypes/perf.yaml-  - asllabeling
--
rules/datatypes/eeg.yaml:- suffixes:
rules/datatypes/eeg.yaml-  - eeg
--
rules/datatypes/eeg.yaml:- suffixes:
rules/datatypes/eeg.yaml-  - channels
--
rules/datatypes/eeg.yaml:- suffixes:
rules/datatypes/eeg.yaml-  - coordsystem
--
rules/datatypes/eeg.yaml:- suffixes:
rules/datatypes/eeg.yaml-  - electrodes
--
rules/datatypes/eeg.yaml:- suffixes:
rules/datatypes/eeg.yaml-  - events
--
rules/datatypes/eeg.yaml:- suffixes:
rules/datatypes/eeg.yaml-  - photo
--
rules/datatypes/func.yaml:- suffixes:
rules/datatypes/func.yaml-  - bold
--
rules/datatypes/func.yaml:- suffixes:
rules/datatypes/func.yaml-  - phase  # deprecated
--
rules/datatypes/func.yaml:- suffixes:
rules/datatypes/func.yaml-  - events
--
rules/datatypes/func.yaml:- suffixes:
rules/datatypes/func.yaml-  - physio
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - T1w
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - T1map
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - defacemask
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - MESE
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - VFA
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - IRT1
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - MP2RAGE
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - MPM
--
rules/datatypes/anat.yaml:- suffixes:
rules/datatypes/anat.yaml-  - MTR
--
rules/datatypes/fmap.yaml:- suffixes:
rules/datatypes/fmap.yaml-  - phasediff
--
rules/datatypes/fmap.yaml:- suffixes:
rules/datatypes/fmap.yaml-  - epi
--
rules/datatypes/fmap.yaml:- suffixes:
rules/datatypes/fmap.yaml-  - TB1DAM
--
rules/datatypes/fmap.yaml:- suffixes:
rules/datatypes/fmap.yaml-  - TB1EPI
--
rules/datatypes/fmap.yaml:- suffixes:
rules/datatypes/fmap.yaml-  - TB1AFI
--
rules/datatypes/fmap.yaml:- suffixes:
rules/datatypes/fmap.yaml-  - TB1SRGE
--
rules/datatypes/fmap.yaml:- suffixes:
rules/datatypes/fmap.yaml-  - TB1map
--
rules/datatypes/dwi.yaml:- suffixes:
rules/datatypes/dwi.yaml-  - dwi
--
rules/datatypes/dwi.yaml:- suffixes:
rules/datatypes/dwi.yaml-  - sbref
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - meg
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - meg
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - meg
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - headshape
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - markers
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - coordsystem
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - channels
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - events
--
rules/datatypes/meg.yaml:- suffixes:
rules/datatypes/meg.yaml-  - photo

Is there any heuristic for when uppercase and when lowercase is used? Going by bold and cbv I assumed it was always lowercase for the sake of simplicity.

@Remi-Gau
Copy link
Collaborator

Is there any heuristic for when uppercase and when lowercase is used?

AFAICT not really.

Although the all uppercase from quantitative MRI kinda help identify file collections.

But there have been mixed cases (like T1w) since the beginning of BIDS.

@mariehbourget
Copy link
Collaborator Author

Hi @effigies!
I was able to fix the broken link in src/schema/objects/top_level_files.yaml and the RTD build is now fine.

I'm still unable to build the doc locally, see #881 (comment). I followed these instructions in a fresh environment with a fresh clone.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Jan 18, 2022

OK everyone! Stand back!

We are merging!!!

@Remi-Gau Remi-Gau merged commit 6a60eb2 into master Jan 18, 2022
@mariehbourget
Copy link
Collaborator Author

Awesome!! 🎉
Thank you so much to everyone who contributed to this work, you are amazing! ❤️
And thanks to @bids-standard/maintainers for their help and support throughout this process!

@jcohenadad
Copy link
Contributor

i second @mariehbourget , thank you very much to everyone helping. Longue vie à BIDS! 🙌

@Remi-Gau
Copy link
Collaborator

For BEP merge: we should have a BEP merge party with a countdown and have a github action that sends a bottle of champagne (or other beverage of their liking) to the contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BIDS for microscopy (BEP031)