Conversation
Automated Review URLs |
|
@dstansby updated the schemas! Could I ask you to have a swift look if something strikes you as off? |
|
I had a look, and I'm struggling to read the schemas by eye, and the rendering doesn't look so nice either (https://ngff-spec--67.org.readthedocs.build/en/67/schemas/scene/, https://ngff-spec--67.org.readthedocs.build/en/67/schemas/coordinate-transformations/, https://ngff-spec--67.org.readthedocs.build/en/67/schemas/coordinate-systems/) Perhaps a better test than manual review is checking that the examples all validate correctly using the schemas? |
I feel you 🙄
Tbf, it doesn't look very different from the rendering on the current main. I think it's getting especially nasty for the But you're right about the validator - I guess the easiest thing is then to create fork of the validator, point it to this branch for the schemas and check the examples 👍 OR simply copy some of the examples to the testing CI in here. |
There was a problem hiding this comment.
I had more of a look at the RTD build, and some initial comments:
There's "Coordinate systems" and "Coordinate systems and transforms" pages, which seems confusing (duplication of coordinate systems):

I don't understand why coordinate systems and transformations are in the same schema, wouldn't it be much cleaner to have them in separate schemas?
On a similar theme, it would be much nicer to have all the different transforms as their own schema that are then referenced by other schemas. It would incraese the number of schema files, but make it much easier to find a given transformation for example.
I think that's a legacy thing. They maybe used to be but the schemas are separated as you suggest. It was just that the title of the coordinate transformations schema file was still "coordinate systems and transformations" - I changed it, should be fixed now.
Agree! Makes total sense and can do. |
|
Yep, no problem 👍 |
|
@dstansby I added a bunch of the examples from jo-mueller/ngff-rfc5-coordinate-transformation-examples to the test cases here. They all check out alright, except for one |
Co-authored-by: David Stansby <dstansby@gmail.com>
| whose values MUST be an array of valid [coordinate systems](#coordinate-systems-md). | ||
|
|
||
| If used inside "scene" metadata, the `input` and `output` fields of `coordinateTransformations` MUST contain a json object, | ||
| which MUST contain either the `path` or the `name` field, or both. |
There was a problem hiding this comment.
The name is always required for input/output. Can't have just path.
There was a problem hiding this comment.
If NOT used inside a "scene" the coordinateTransform input and output fields can be strings OR objects.
If NOT used inside a "scene" the schema also allows input and output to be empty objects (since neither "name" nor "path" are required). Is this correct or does the schema need fixing?
These details aren't mentioned here.
There was a problem hiding this comment.
@will-moore good catch, the name must always be there. I am a bit hesitant to make changes to the text here because I was planning for this PR to only mirror the latest RFC5 proposal. I'll create an issue on ngff to make sure we don't forget later?
|
@dstansby found it! The failing example ( |
Otherwise, axes MUST have two or three spatial axes, which is forbidden for multiscales coordinate systems
This is the PR following up to ome/ngff#389.
This PR
PRs to merge first: