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

562 update building nonstructural damage to support flood #571

Merged

Conversation

longshuicy
Copy link
Member

@longshuicy longshuicy commented May 13, 2024

This PR:

  1. Refactor nonstructural building damage to accept one fragility key at time and run damage analysis
  2. Add support for flood
  3. Rewrite pytest example of AS and DS
  4. Add pytest for flood; remove flood example from building damage

Related PR:
incore-docs: IN-CORE/incore-docs#386

@navarroc
Copy link
Member

If I set use_liquefaction and a liquefaction_dataset_id with the flood test, I get an error. I think we have to check to make sure the hazard type is EQ or ignore the liquefaction as we do in building damage.

Copy link
Member

@navarroc navarroc left a comment

Choose a reason for hiding this comment

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

Looks good. Compared the combined damage before and after the change, results match. Also verified setting liquefaction with the flood hazard works correctly.

Copy link
Member

@ywkim312 ywkim312 left a comment

Choose a reason for hiding this comment

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

I got this error when I ran test_buildingdamage_legacy
requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://incore-dev.ncsa.illinois.edu/data/api/datasets/6091d5a8daa06e14ee96d502
Could you check? test_buildingdamage ran okay

@ywkim312
Copy link
Member

All other tests were fine except test_buildingdamage_legacy

@navarroc
Copy link
Member

It looks like the retrofit strategy referred to in the test got deleted, I saw the same for road damage, the galveston road inventory referenced is gone. We need some way to mark/protect these test files. This has happened too many times.

@navarroc
Copy link
Member

Non structural damage needs to support retrofit_strategy (like building damage). I left a comment on the docs PR, but we should either include that in this PR or open a bug to fix it in a separate PR.

@longshuicy
Copy link
Member Author

@navarroc @ywkim312 sorry for the delay, this is ready to be reviewed again. I also updated the incore-docs PR.

Copy link
Member

@navarroc navarroc left a comment

Choose a reason for hiding this comment

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

Tested using test classes and IN-CORE/incore-docs#394

Copy link
Member

@ywkim312 ywkim312 left a comment

Choose a reason for hiding this comment

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

All the tests were passed. Approve

@ywkim312 ywkim312 merged commit 45750d7 into develop May 24, 2024
7 checks passed
@ywkim312 ywkim312 deleted the 562-update-building-nonstructural-damage-to-support-flood branch May 24, 2024 18:51
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.

Update building nonstructural damage to support flood
3 participants