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

[ENH] Microscopy: NGFF format support #1104

Merged
merged 10 commits into from
Jul 7, 2022
Merged

[ENH] Microscopy: NGFF format support #1104

merged 10 commits into from
Jul 7, 2022

Conversation

TheChymera
Copy link
Collaborator

@TheChymera TheChymera commented May 19, 2022 •
edited by sappelhoff
Loading

@TheChymera TheChymera requested a review from tsalo as a code owner May 19, 2022 17:23
@codecov
Copy link

codecov bot commented May 19, 2022 •
edited
Loading

Codecov Report

Merging #1104 (988bd2d) into master (24dbe58) will increase coverage by 0.80%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #1104      +/-   ##
==========================================
+ Coverage   75.20%   76.00%   +0.80%     
==========================================
  Files          11       11              
  Lines        1359     1363       +4     
==========================================
+ Hits         1022     1036      +14     
+ Misses        337      327      -10
Impacted Files Coverage Δ
tools/schemacode/schemacode/validator.py 83.66% <87.50%> (+0.55%) ⬆️
...ools/schemacode/schemacode/tests/test_validator.py 100.00% <100.00%> (ø)
tools/schemacode/schemacode/_version.py 38.90% <0.00%> (+2.59%) ⬆️

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 24dbe58...988bd2d. Read the comment docs.

@TheChymera TheChymera mentioned this pull request May 19, 2022
Merged
@mariehbourget
Copy link
Collaborator

This looks good, thanks @satra and @TheChymera for this initiative!

Regarding the example and validation, there are 4 JSON fields that are currently checked for consistency between the JSON metadata and the OME-TIFF metadata:
PixelSize, Immersion, NumericalAperture and Magnification.

Is NGFF using the same OME data model? If so, it may require to update the description of those fields and the validator accordingly.

effigies
effigies previously approved these changes May 23, 2022
View reviewed changes
yarikoptic
yarikoptic reviewed May 24, 2022
View reviewed changes
Copy link
Collaborator

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

I have a concern of using .ngff/ which is not reflective of the underlying container (zarr) and seems not used by OME NGFF community itself.

src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
src/schema/objects/extensions.yaml Outdated Show resolved Hide resolved
@yarikoptic yarikoptic mentioned this pull request May 24, 2022
Closed
@joshmoore joshmoore mentioned this pull request May 25, 2022
Closed
@joshmoore
Copy link

mariehbourget commented 5 days ago
Is NGFF using the same OME data model? If so, it may require to update the description of those fields and the validator accordingly.

@mariehbourget, the latest versions of NGFF will also support the same OME-XML as is embedded in OME-TIFF so for those fields, the same validation would be feasible. However, there's added complication that some values on your list like PixelSize also have a representation in NGFF. They are intended to be the same value in two representations, but you might decide to cross-validate.

@effigies effigies dismissed their stale review May 25, 2022 15:28

Pending decision about extension

tsalo
tsalo reviewed May 27, 2022
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.

There appears to be a small typo in the updated text, but other than that this looks good to me.

src/04-modality-specific-files/10-microscopy.md Outdated Show resolved Hide resolved
@sappelhoff sappelhoff changed the title NGFF format support [ENH] NGFF format support Jun 8, 2022
@sappelhoff sappelhoff added this to the 1.7.1 milestone Jun 8, 2022
sappelhoff
sappelhoff reviewed Jun 8, 2022
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.

@TheChymera I see a corresponding examples PR (bids-standard/bids-examples#315), but no validator PR. Could you please open one?

I also tagged Remi and Marie-Helene to give this a review.

@TheChymera
Copy link
Collaborator Author

@sappelhoff on it, needs a bit more work since thus far paths are all treated as files, so validating directories needs to be built into the logic.

sappelhoff
sappelhoff reviewed Jun 9, 2022
View reviewed changes
src/CHANGES.md Outdated Show resolved Hide resolved
TheChymera and others added 5 commits June 9, 2022 05:11
@TheChymera @satra
NGFF format support
656b10f 
Co-authored-by: Satrajit Ghosh <satrajit.ghosh@gmail.com>
@satra @yarikoptic @TheChymera
Update src/schema/rules/datatypes/micr.yaml
e22c916 
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@satra @yarikoptic @TheChymera
Update src/schema/objects/extensions.yaml
266effb 
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@satra @yarikoptic @TheChymera
Update src/04-modality-specific-files/10-microscopy.md
a9403fc 
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@TheChymera
typo
64d7b27
@TheChymera
Copy link
Collaborator Author

@sappelhoff there we go :) damage should be undone now.

@TheChymera
Copy link
Collaborator Author

TheChymera commented Jun 9, 2022 •
edited
Loading

no validator PR. Could you please open one?

Should be handled by: 4fb2574

@TheChymera
Copy link
Collaborator Author

Also have a PR for the production JS validator: https://github.com/bids-standard/bids-validator/pull/1467

tsalo
tsalo reviewed Jun 9, 2022
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 to the schema and specification look good to me. The core changes to the code also make sense to me, but I think we could change some of the wording to make it easier to understand.

tools/schemacode/schemacode/validator.py Outdated Show resolved Hide resolved
tools/schemacode/schemacode/validator.py Outdated Show resolved Hide resolved
tools/schemacode/schemacode/validator.py Show resolved Hide resolved
tools/schemacode/schemacode/validator.py Show resolved Hide resolved
tools/schemacode/schemacode/validator.py Outdated Show resolved Hide resolved
@TheChymera
Copy link
Collaborator Author

@tsalo ok, ready :)

@TheChymera TheChymera requested a review from tsalo June 10, 2022 15:13
tsalo
tsalo approved these changes Jun 10, 2022
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.

LGTM. Thanks!

mariehbourget
mariehbourget reviewed Jun 10, 2022
View reviewed changes
Copy link
Collaborator

@mariehbourget mariehbourget 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 looks good!
I have a few suggestions for consistency with the OME-TIFF formats.

When we worked on the BEP, we were asked to validate consistency of metadata that were both present in the file metadata and in the json sidecar metadata.
In the current spec, this consistency validation is done for PixelSize, Immersion, Magnification and NumericalAperture, (see previous comment here #1104 (comment))

It was confirmed that those fields are also present in NGFF (#1104 (comment)).

I don't know if this type of validation is possible right now with NGFF in javascript in the validator but I wanted to mention it.
If the validation is done, then the descriptions of these fields would also need to be updated to match in metadata.yaml.

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

@mariehbourget I added a new commit expanding the explanation of OME-ZARR (aka NGFF).

@chrisgorgo
@CPernet
@Remi-Gau

looks good?

@effigies effigies merged commit f2e34d4 into bids-standard:master Jul 7, 2022
@TheChymera TheChymera deleted the ngff branch July 7, 2022 19:09
bids-maintenance added a commit that referenced this pull request Jul 7, 2022
@bids-maintenance
[DOC] Auto-generate changelog entry for PR #1104
0332bb4
@TheChymera TheChymera mentioned this pull request Jul 8, 2022
Merged
TheChymera pushed a commit to TheChymera/bids-specification that referenced this pull request Jul 13, 2022
@bids-maintenance @TheChymera
[DOC] Auto-generate changelog entry for PR bids-standard#1104
be5ff0c
@sappelhoff sappelhoff changed the title [ENH] NGFF format support [ENH] Microscopy: NGFF format support Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariehbourget mariehbourget mariehbourget left review comments

@sappelhoff sappelhoff sappelhoff left review comments

@satra satra satra left review comments

@joshmoore joshmoore joshmoore left review comments

@yarikoptic yarikoptic yarikoptic left review comments

@tsalo tsalo tsalo approved these changes

@effigies effigies effigies approved these changes

@Remi-Gau Remi-Gau Awaiting requested review from Remi-Gau

@chrisgorgo chrisgorgo Awaiting requested review from chrisgorgo

@CPernet CPernet Awaiting requested review from CPernet

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

Successfully merging this pull request may close these issues.
8 participants
@TheChymera @mariehbourget @joshmoore @yarikoptic @effigies @satra @tsalo @sappelhoff