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 IMT.csv #259

Open
wants to merge 6 commits into
base: 0.3-codelist-updates
Choose a base branch
from
Open

Update IMT.csv #259

wants to merge 6 commits into from

Conversation

matamadio
Copy link
Contributor

@matamadio matamadio commented Sep 26, 2023

Add imt code for cyclone wind speed 10-min sustained. It was already there as km/h but STORM uses m/s...

This makes me reconsider the discussion in #5, and splitting IMT into two separate codelists:

  • metric (optional)
  • unit (mandatory)

So even if the specific metric (larger range of variability) is not in the codelist, one can still indicate the unit if part of the most common ones (smaller range of variability).

Related issues

#5

Merge checklist

If you added, removed or renamed a field:

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

Always:

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

Having trouble?

See how to resolve check failures.

Add imt code for cyclone wind speed 10-min sustained. It was already there as km/h but STORM uses m/s...

This makes me reconsider the discussion in #5, and splitting IMT into two separate codelists:
- metric (optional)
- unit (mandatory)

So even if the specific metric (larger range of variability) is not in the codelist, one can still indicate the unit if part of the most common ones (smaller range of variability).
@matamadio
Copy link
Contributor Author

In addition: it's not always useful to specify the hazard type in the metric. In some cases it just create unnecessary duplication when the same metric:unit could be used.

E.g.
Why we need to differentiate FL from LS when speaking of flow depth, velocity?
How tsunami inundation depth is different from FL or LS "flow depth"?
Why we need to indicate "etc" (extra-tropical cyclones) for strong wind unit?

schema/codelists/open/IMT.csv Outdated Show resolved Hide resolved
@odscjen
Copy link
Contributor

odscjen commented Oct 9, 2023

The other tests that are failing are because you've added some codes to closed codelists (metric_dimensions, and exposure_category) but you've not included the new codes in every instance of enum that uses that codelist. (this was presumably in a different PR)

@matamadio
Copy link
Contributor Author

matamadio commented Oct 9, 2023

The other tests that are failing are because you've added some codes to closed codelists (metric_dimensions, and exposure_category) but you've not included the new codes in every instance of enum that uses that codelist. (this was presumably in a different PR)

I've only edited this open codelist. Still, I don't know how to do what you are suggesting: which file should be edited in addition to csvs?

Copy link
Contributor Author

@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.

Not sure about the edits in metric_dimension.csv
I see the change of removing quotation marks on line 7, but other lines have quotation marks as well..?

Added custom and commercial licenses - no urls
Add "description" to _source_ attributes.
@odscjen
Copy link
Contributor

odscjen commented Oct 17, 2023

Hi Mat you've got 3 errors showing up I'll deal with them in turn.

  1. One or more codelist CSV files are invalid

The problem is that lines 11 and 12 in licence.csv both end with a space, they should just end with the comma. It looks like that error was introduced in this commit 6e35b0b

  1. Files are not indented as expected

This one can be fixed by running ocdskit indent -r . as explained in the developer docs You have to run this locally on your own computer, it can't be done within github.

  1. One or more JSON Schema files are invalid

I've had a bit more of an investigate and the problem is again in an older commit to this base branch. It looks like Stu made a commit ba9472e earlier that added 2 new codes to the exposure_category.csv. However he only updated the csv file itself. For closed codelists (like this one) the codes also need to match those in enum fields in the schema itself.

e.g.

"category": {
          "title": "Asset category",
          "description": "The category of the lost assets, from the closed [exposure_category codelist](https://docs.riskdatalibrary.org/en/{{version}}/reference/codelists/#exposure-category).",
          "type": "string",
          "codelist": "exposure_category.csv",
          "openCodelist": false,
          "enum": [
            "agriculture",
            "buildings",
            "infrastructure",
            "population",
            "natural_environment"
          ]
        },

in this example (which is from Losses) only the original 5 codes are listed out but there's now 7 codes in the referenced file so the test expects to see those same 7 codes in the enum field where codelist references "exposure_category.csv".

@odscjen
Copy link
Contributor

odscjen commented Oct 17, 2023

I see the change of removing quotation marks on line 7, but other lines have quotation marks as well..?

Ah yes you only need quotation marks if there are special characters (normally commas). If there aren't any, as is the case in that line of metric_dimensions.csv, the tests complain.

@matamadio
Copy link
Contributor Author

I think I solved some of the PR errors, but others remain:

  • json indent (can't locate the issue, if exists)
  • properties within properties (ignore?)

@odscjen
Copy link
Contributor

odscjen commented Oct 30, 2023

properties within properties can be safely ignored, it's just a warning rather than an actual error.

indent errors are almost impossible to spot by eye and requires the dev env to fix it using the ocdskit module. It's one of the tests that can be safely turned off for the time being if that's what you'd like?

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.

2 participants