-
Notifications
You must be signed in to change notification settings - Fork 3
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
WMTSCapabilitiesGenerator Class #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take my review with a grain of salt, as I don't have all of the context and am not super familiar with WMTS XML schema, but I think this is looking pretty good!
Just a couple of suggestions and a callout for a (I think) mistakenly added file. Otherwise looks good to me :)
23ad27e
to
2fd5a65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of follow-up comments. I think the only thing I would require for an approval here is addressing the comment about adding an explanation around why the left/right bounds limit do not go all the way to -180/180.
If you have time to do so, I think adding a unit test would also be great and make it more clear that this behaves as desired. Instantiate this class with some known inputs and assert that an xml document can be created. Then check that key elements are structured properly in the output xml. Perhaps you could use something like https://github.com/geopython/OWSLib to validate the resulting capabilities document?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments!
I still think some explanatory comments in the code for some of the confusions around differences with left/right bounds would be useful. Right now we have a team that's familiar with the limitations and expectations around this, but I would suggest assuming that this won't always be the case! People forget/leave, and new people to this project may have to re-ask questions that have already been answered. Leaving comments explaining peculiarities like this go a long way toward long-term maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alonakos, I like the way that you structured this class, I found it easy to understand and use. Appreciate the usage example in the docstring especially!
I tested it out and it generates valid XML for the TMS that we use by default, WGS84Quad
. However, when I tested with other TMSs, I realized that some of the values in the generated XML are from defaults in the class, when they should actually be taken from the TMS definition itself. For example, if we create a WMTSCapabilitiesGenerator
with tile_matrix_set="UPSArcticWGS84Quad"
, some of the properties don't match the TMS definition:
<TileMatrixSet xml:id="WorldCRS84Quad">
<ows:Title>CRS84 for the World</ows:Title>
<ows:Identifier>UPSArcticWGS84Quad</ows:Identifier>
<ows:BoundingBox crs="http://www.opengis.net/def/crs/OGC/1.3/CRS84">
<ows:LowerCorner>-180 -90</ows:LowerCorner>
<ows:UpperCorner>180 90</ows:UpperCorner>
</ows:BoundingBox>
the xml:id
should be UPSArcticWGS84Quad
, the ows:Title
should be Universal Polar Stereographic WGS 84 Quad for Arctic
, and the bounding box CRS should be "http://www.opengis.net/def/crs/EPSG/0/5041"
, and the BB coordinates should be in the CRS of the TMS.
Something that might be useful is to have a look at the TMS definitions in XML to compare exactly what the XML should look like for each TMS. (I just discovered these XML examples while doing this review!) I think there may be extra properties to add for some TMS's as well. Helpfully, the same file directory includes definitions for the different parts of the WMTS capabilities document as well, if needed.
The code is well organized so hopefully these adjustments will be fairly straight forward to implement! Let me know if you want to discuss anything. :)
A class to generate WMTS Capabilities XML for a given dataset.
See discussion: PermafrostDiscoveryGateway/viz-workflow#28