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

Added Power to latest Pelicun #90

Merged
merged 21 commits into from
Jan 28, 2025
Merged

Conversation

snaeimi
Copy link
Contributor

@snaeimi snaeimi commented Dec 31, 2024

I intentionally deleted my fork of PELICUN which was forked from Adam's forked. So, all me contribution there is gone. I reforked the repor from NHERI-SimCenter/pelicun tried to figure out which changes translates to my forked version. These chanegs are not yet tested. Also needs the pull request for DBDL repo to be accepted

I intentionally deleted my fork of PELICUN which was forked from Adam's forked. So, all me contribution there is gone. I reforked the repor from NHERI-SimCenter/pelicun tried to figure out which changes translates to my forked version. These chanegs are not yet tested. Also needs the pull request for DBDL repo to be accepted
Copy link
Collaborator

@zsarnoczay zsarnoczay left a comment

Choose a reason for hiding this comment

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

Thanks for these updates, Sina. They generally look great, but I added a few comments asking for minor edits. Please take a look and let me know your thoughts.

A bigger, general issue is that ruff checks are failing currently. If you are already checking with ruff before sending a pull request, then you might not be checking with the right version. Unfortunately, the rules change slightly between ruff versions and the only way we found to be consistent with our checks across developers is to anchor them to a particular ruff version. You can see which ruff version we use by looking at the development dependencies in setup.py: https://github.com/NHERI-SimCenter/pelicun/blob/master/setup.py
An alternative, perhaps easier, approach is to set up a virtual env for testing pelicun and have only pelicun installed there with development dependencies. This way you can always be sure to test with the right dependencies.

There's also quite a few spelling issues or typos in the script you prepared. Please take a look at the coding practice in the pelicun docs (https://nheri-simcenter.github.io/pelicun/developer_guide/code_quality.html) and make sure you run the spell checking (3.1.3) to avoid such trivial errors.

I know this might sound like a lot of overhead. I ask you to do it because it quickly becomes a simple task and leads to much better code quality. Thanks.

pelicun/resources/auto/Hazus_Earthquake_IM.py Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
snaeimi and others added 7 commits January 6, 2025 17:07
@snaeimi
Copy link
Contributor Author

snaeimi commented Jan 9, 2025

I have Linted and checked this changes. Ready to be pulled

@snaeimi snaeimi requested a review from zsarnoczay January 9, 2025 21:43
Copy link
Collaborator

@zsarnoczay zsarnoczay left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and improvements!

Unfortunately, the spell checker does not seem to pick up all the typos in this script. So, I'll point them out with short messages like this:
Typo: "itw as"

When fixing typos please make sure to replace all instances of them in the script - I'll only point out each the first time I see it. And, since now we know the spell checker is far from perfect, please spend some time checking your script for typos before you commit - it takes a lot of effort for me to review and point out all of these.

I also wanted to comment on your desire to serve various inputs from users. I agree this is a positive and supportive attitude and I do not mean to force you to constrain inputs unless you agree it make sense to you. But keep in mind that by providing a lot of options, you also create ambiguity. With many options, users can think that you can deal with any input that "makes sense". This is not helpful and they will run into problems very quickly despite your best efforts. It is usually enough to provide a strict, but reasonable schema for the inputs and it should be perfectly fine to prescribe values to be able to cast to a certain type (such as int or bool). Values should not magically change types throughout the workflow, so that should not be a concern. It is also easier to expand and edit the scripts in the future if they are simpler.
Having said all of that, if you wish to support all kinds of inputs, I am not going to stop you.

Finally, I wanted to comment on the default values and implicit inference. These are all over the auto population functions today. We are in the process of separating inference from mapping; that is inference being the part where you plug in gaps by infering missing features (such as anchors in this case) and mapping where you already know every important feature and aim to pick the right damage model based on them. These scripts will only include the mapping portion and there won't be any inference in them. All of the inference will move to BRAILS++. I tell you this as a heads-up so that you are aware of what is coming; it will take a few months for us to replace every auto-pop script - I expect May 2025 to be a reasonable target.

Overall, most of my comments are about the typos, the remaining are a few suggestions on how to make the code easier to read and extend.

Please revise and ask for re-review when done. Thanks.

pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
pelicun/resources/auto/Hazus_Earthquake_IM.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 1.12994% with 175 lines in your changes missing coverage. Please review.

Project coverage is 78.50%. Comparing base (80cfc9b) to head (95121a4).
Report is 195 commits behind head on develop.

Files with missing lines Patch % Lines
pelicun/resources/auto/Hazus_Earthquake_IM.py 1.12% 175 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #90      +/-   ##
===========================================
- Coverage    83.81%   78.50%   -5.32%     
===========================================
  Files           16       17       +1     
  Lines         4572     5061     +489     
===========================================
+ Hits          3832     3973     +141     
- Misses         740     1088     +348     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zsarnoczay zsarnoczay merged commit 46c0e0f into NHERI-SimCenter:develop Jan 28, 2025
15 checks passed
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.

3 participants