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

update descriptions for spatial, bbox, coordinates and centroid #207

Merged
merged 12 commits into from
Aug 29, 2023

Conversation

odscjen
Copy link
Contributor

@odscjen odscjen commented Aug 24, 2023

Related issues

closes #197 and closes #201

Description

Merge checklist

  • Update the changelog (style guide)
  • Run ./manage.py pre-commit

If you added or removed a field:

  • Update the collapse option of the jsonschema directives for dataset, resource, hazard, exposure, vulnerability and loss on reference/schema.md

Having trouble?

See how to resolve check failures.

@odscjen odscjen requested a review from duncandewhurst August 24, 2023 15:07
Copy link
Contributor

@matamadio matamadio left a comment

Choose a reason for hiding this comment

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

Edit:

The geographical area covered by the dataset. The use of bbox is recommended over geometry or centroid.

A geographic bounding box defined by two longitudes and two latitudes delimiting the geographical area. Values must be given using the World Geodetic System 1984 (WGS 84) [WGS84] datum, with longitude and latitude units of decimal degrees, e.g. bbox = [51.2867;51.6918;-0.5103;0.3340]

Is there any particular order to give the 4 values? e.g.
bbox = min Longitude , min Latitude , max Longitude , max Latitude?
If so, must be specified here.

One or more GeoJSON positions according to the GeoJSON geometry type defined in .type. Values must be given using the World Geodetic System 1984 (WGS 84) [WGS84] datum, with longitude and latitude units of decimal degrees."

  • Please add example
  • Specify a limit to coordinate pairs (e.g. 30 or 50)

@odscjen
Copy link
Contributor Author

odscjen commented Aug 25, 2023

@matamadio I've added the specification of the order of the 4 values in bbox and an example to .coordinates. There is no limit to the number of coordinates that can be provided so I've not changed this.

@odscjen odscjen requested a review from matamadio August 25, 2023 09:55
Copy link
Contributor

@matamadio matamadio left a comment

Choose a reason for hiding this comment

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

I would add specific coordinate examples for all fields, just to be safe.
Also specify that coordinates come in "comma separated values" and not semicolon or other symbols.

e.g. bbox = (51.2867,-0.5103,51.6918,0.3340)

e.g. centroid = (51.751944, -1.257778)

e.g. polygon = (...)

I understand that there are no explicit limitations for coordinates number, but again to be safe I would discourage from adding very complex geometries that could result into large files / brake the CoVE tool. Maybe just specify "Please use simplified geometries"?

@odscjen
Copy link
Contributor Author

odscjen commented Aug 25, 2023

Also specify that coordinates come in "comma separated values" and not semicolon or other symbols.

The problem with doing this is that this is how it should be put in in JSON but in the spreadsheet template it needs to be semi-colon. The spreadsheet template is generated automatically from the JSON schema pulling the field descriptions into the '# description' row. So if we explicitly say to use CSV values here the relevant columns in the spreadsheet will end up with conflicting guidance.

But we can fit it somewhere into the guidance documentation so I've added a note to the documentation issue.

@odscjen odscjen requested a review from matamadio August 25, 2023 13:12
Copy link
Contributor

@duncandewhurst duncandewhurst left a comment

Choose a reason for hiding this comment

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

We may as well make the schema change proposed in #197 (comment) since it is an improvement and it is just a case of copy-pasting.

schema/rdls_schema.json Outdated Show resolved Hide resolved
schema/rdls_schema.json Outdated Show resolved Hide resolved
schema/rdls_schema.json Outdated Show resolved Hide resolved
schema/rdls_schema.json Outdated Show resolved Hide resolved
@odscjen
Copy link
Contributor Author

odscjen commented Aug 28, 2023

We may as well make the schema change proposed in #197 (comment) since it is an improvement and it is just a case of copy-pasting.

But that thread ends with #197 (comment) where we agreed the only change that needed made was to update the descriptions?

But I do agree that tightening up the constraints would be a good thing so I'll add that update in anyway

@odscjen odscjen requested a review from duncandewhurst August 28, 2023 08:24
@duncandewhurst
Copy link
Contributor

But that thread ends with #197 (comment) where we agreed the only change that needed made was to update the descriptions?

Yes, sorry. I meant to include implementing the schema change since it is already worked out!

Copy link
Contributor

@duncandewhurst duncandewhurst left a comment

Choose a reason for hiding this comment

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

One unresolved comment from my previous review. Otherwise, looks good.

@odscjen odscjen dismissed duncandewhurst’s stale review August 29, 2023 10:27

changes have now been applied

@odscjen odscjen merged commit 990e3ad into dev Aug 29, 2023
@odscjen odscjen deleted the 197-spatial-coordinates branch August 29, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants