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 Vulnerability adding impact object, function objects and changing some field names and descriptions #124

Merged
merged 31 commits into from
Jul 7, 2023

Conversation

odscjen
Copy link
Contributor

@odscjen odscjen commented Jul 4, 2023

Related issues

closes #74, closes #75, closes #76, closes #77, closes #78, closes #125 and addresses some of #56

Merge checklist

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

@odscjen odscjen marked this pull request as draft July 4, 2023 11:59
@odscjen
Copy link
Contributor Author

odscjen commented Jul 4, 2023

required #101 to be approved and merged before this one is ready as it contains the Cost object that is created in #101

@odscjen odscjen marked this pull request as ready for review July 4, 2023 15:13
@odscjen
Copy link
Contributor Author

odscjen commented Jul 4, 2023

impact_metric.csv still requires descriptions, see #125 for that issue but it shouldn't hold up pushing this PR through

schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
codelists/closed/spatial_scale.csv Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Show resolved Hide resolved
@duncandewhurst
Copy link
Contributor

Also, need to remove unreferenced definitions:

  • common_aggregation_type
  • common_occupancy_time
  • vulnerability_f_relationship

@duncandewhurst duncandewhurst mentioned this pull request Jul 5, 2023
2 tasks
@odscjen odscjen requested a review from duncandewhurst July 6, 2023 12:36
@odscjen odscjen mentioned this pull request Jul 6, 2023
3 tasks
Copy link
Member

@stufraser1 stufraser1 left a comment

Choose a reason for hiding this comment

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

Please see individual comments

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.

@stufraser1 please take a look at my review comments, there are quite a few on which it would be good to get your input. If you are happy with any of the suggestions, feel free to commit them. Thanks!

codelists/open/classification_scheme.csv Outdated Show resolved Hide resolved
codelists/open/classification_scheme.csv Outdated Show resolved Hide resolved
codelists/open/impact_metric.csv Outdated Show resolved Hide resolved
codelists/open/impact_metric.csv Outdated Show resolved Hide resolved
codelists/open/impact_metric.csv Outdated Show resolved Hide resolved
codelists/open/impact_metric.csv Outdated Show resolved Hide resolved
codelists/open/impact_metric.csv Outdated Show resolved Hide resolved
codelists/open/impact_metric.csv Outdated Show resolved Hide resolved
codelists/closed/hazard_type.csv Outdated Show resolved Hide resolved
@odscjen odscjen requested a review from stufraser1 July 7, 2023 08:48
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to add a code:
asset_count,Number of assets damaged,"The estimated number of structure, or length of infrastructure damaged.", Loss
If you think we need to separate count and length (for network infrastructure we communicate in terms of km of roads damaged e.g.) then please suggest the code suffix.

Copy link
Contributor Author

@odscjen odscjen Jul 7, 2023

Choose a reason for hiding this comment

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

Because this list is used in impact which has .metric (which takes from this list) and .unit I don't think we need to split this up into two as the unit should clarify it. But I have changed the name to 'asset_loss' to retain _count for when metrics only relate to a count of numbers.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to add a code:
displaced_count,Number of people displaced,"The estimated number of people displaced due to physical damage and disruption.", Loss

Copy link
Member

Choose a reason for hiding this comment

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

Can we make the category "Loss and Damage" instead of "Loss"?
This would be very helpful to communicate the range of impacts not just being monetary Loss, but I don't know if this change would have other implications in the standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The category should refer to the object the code can be used in. At the moment that's Loss. I see that you've suggested a change from Loss to Loss_and_damage, I feel that's a change that is best left until the next iteration of the standard as that feels like quite a significant semantic change.

volcano_balllistics
changed to 
volcano_ballistics
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem comprehensive. Why do we not have GED4ALL taxonomy for buildings referenced here, for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So at the moment the only place this codelist is being referenced is in se_category so we only included the schemes for that

@odscjen odscjen requested a review from stufraser1 July 7, 2023 11:30
Copy link
Member

@stufraser1 stufraser1 left a comment

Choose a reason for hiding this comment

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

Agree on putting semantic change to SC review

@odscjen odscjen dismissed duncandewhurst’s stale review July 7, 2023 12:31

requested changes applied

@odscjen odscjen merged commit 6dc27bf into dev Jul 7, 2023
@odscjen odscjen deleted the vulnerability branch July 7, 2023 12:31
@odscjen odscjen restored the vulnerability branch July 7, 2023 14:15
@odscjen odscjen deleted the vulnerability branch July 7, 2023 14:50
@stufraser1
Copy link
Member

If this was merged in dev branch, why does the json file on dev branch still contain outdated codes e.g.,
enum": [
"Agriculture",
"Buildings",
"Infrastructures",
"Population",
"Natural environment"
]

@duncandewhurst
Copy link
Contributor

If this was merged in dev branch, why does the json file on dev branch still contain outdated codes e.g., enum": [ "Agriculture", "Buildings", "Infrastructures", "Population", "Natural environment" ]

I think that's just an omission. There's one occurrence of the out-of-date enum (in Loss.category), which I've flagged in #132 (comment).

There's a draft PR (#127) which adds a test to validate that codelist enums are in-sync with the CSV files. I've suggested that we merge that PR after merging the other PRs in order to catch anything we missed.

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