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 #1359

Merged
merged 75 commits into from
Jan 12, 2022
Merged

[ENH] BEP031: Microscopy #1359

merged 75 commits into from
Jan 12, 2022

Conversation

mariehbourget
Copy link
Contributor

@mariehbourget mariehbourget commented Oct 7, 2021

This PR aims to incorporate Microscopy to the bids-validator (BEP031, @mariehbourget, @jcohenadad)

We have encountered some issues with the schema and with an external library and would appreciate some feedback and guidance before we continue.

Questions about schema

We had a working version of the microscopy validator in commit 5d0a27e before we merged recent changes from master (commit f2588ed). If we understand correctly, the bids-validator now uses the yaml files from the schema to validate the datasets. We will make the appropriate changes related to that in order to have a working version again but we have some questions first:

  1. Is the validation through yaml schema files mandatory or should we still be able to run it like the previous version?
  2. When using yaml files, are those files duplicated from the bids-specification/src/schema?
  3. If yes, is there any sort of "synchronisation" between bids-spec and bids-validator or should we update them in both repos?

Note: Some yaml files were not included for Microscopy in bids-spec to facilitate community review. If we have to add them to bids-validator, I would like to add them as well in bids-spec for consistency.

Questions about external libraries

For microscopy, we need to read metadata from the XML header of OME-TIFF files. We are able to do that via the exifreader, esm and xml2js libraries.
However, when installing exifreader with npm, the node_module bids-validator is automatically uninstalled.
We think we might not do the installation correctly or there might be a security issue with xmldom (a dependance of exifreader). Running npm audit gives us this message:

npm_audit

As we are not very experienced with javascript, we would need help with integrating and installing external librairies with the bids-validator. Thank you in advance!

etiennebergeron and others added 29 commits July 28, 2021 14:21
…le 'checkSamples.js' to check if the samples files are in the dataset. WIP; checkSamples considers for now that those files are always needed. Need to add the regex to find the sample entity. Modified 'fulLTest.js' to call 'checkSamples.js'.
…ght not need this file, WIP. In 'microscopy.json', removed the recommended fields from the 'required' dict.
… 'samples.tsv'. checkSamples deleted and its call in fullTest.js as well.
…e files 'fullTest.js', 'tsv.js' and 'validate.js'. I will work on something else and come back later on it.
…me.js'. The file is called in the fullTest only if the dataset has for modality 'Microscopy'.
…g error code: 55 where a JSON field's value is not equal to one of the allowed values, so the code 221 is deleted.
…o manage the JSON name rules separately. Modified 'type.js' to import this new dictionary. Put the <sample> entity optional at the session and top level.
…ds 'Immersion', 'NumericalAperture', 'Magnification', 'ChunkTransformationMatrix' and 'ChunkTransformationMatrixAxis'.
@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #1359 (bd78f58) into master (c1c9615) will decrease coverage by 0.72%.
The diff coverage is 75.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1359      +/-   ##
==========================================
- Coverage   84.83%   84.10%   -0.73%     
==========================================
  Files          83       90       +7     
  Lines        3356     3631     +275     
  Branches     1011     1098      +87     
==========================================
+ Hits         2847     3054     +207     
- Misses        430      483      +53     
- Partials       79       94      +15     
Impacted Files Coverage Δ
bids-validator/utils/files/readBuffer.js 40.00% <40.00%> (ø)
bids-validator/validators/bids/groupFileTypes.js 88.88% <44.44%> (-11.12%) ⬇️
bids-validator/validators/tsv/tsv.js 83.08% <45.09%> (-12.37%) ⬇️
bids-validator/utils/summary/collectModalities.js 91.30% <50.00%> (-1.88%) ⬇️
bids-validator/validators/bids/fullTest.js 95.34% <57.14%> (-3.39%) ⬇️
bids-validator/validators/microscopy/ometiff.js 73.61% <73.61%> (ø)
bids-validator/utils/files/readOMEFile.js 88.88% <88.88%> (ø)
bids-validator/validators/microscopy/validate.js 89.28% <89.28%> (ø)
...tor/validators/microscopy/validateTiffSignature.js 90.00% <90.00%> (ø)
bids-validator/validators/json/json.js 99.11% <94.73%> (-0.89%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1c9615...bd78f58. Read the comment docs.

@mariehbourget
Copy link
Contributor Author

Hi @bids-standard/maintainers,
The implementation of the microscopy validator in this PR is complete and ready for review. 😃

Note: The ExifReader package that we use to read the XML header in OME-TIFF files seems to support only "regular TIFF" files and not "BigTIFF" files. So in cases where the files are "BigTIFF", the consistency checks between the OME metadata and the JSON metadata is skipped with a warning to the user.

Thanks to @etiennebergeron and @konantian for their help with the validator implementation! 🚀

@mariehbourget mariehbourget changed the title [WIP] [ENH] BEP031: Microscopy [ENH] BEP031: Microscopy Jan 3, 2022
Copy link
Member

@rwblair rwblair left a comment

Choose a reason for hiding this comment

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

Only one comment in, in readOEMFile, thoughts on potential memory issues, doesn't necessarily have to be fixed at this time but I wanted to see if any had any opinons/thoughts on it.

Other than that any issues are addressed in this PR:
neuropoly#7

bids-validator/utils/files/readOMEFile.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants