-
Notifications
You must be signed in to change notification settings - Fork 3
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 dsra psra oct2021 (model-factory) #96
Conversation
- added placeholder 'MMI' indicator - changed all_indicators to indicators
- added placeholder 'MMI' indicator - changed all_indicators to indicators - removed vs30lon, vs30lat - fixed prefix sD_Collapse to sDt_Collapse - added CSD aggregation level
- change r2 to r1
- changed all_indicators to indicators - renamed r2 to r1 indicators - added eq risk index calculations / indicators - removed commented out sections of code
- rename r2 to r1 - removed/reordered stat, loss_value from agg_curves_stats - added occupants to avg_loss_stats - removed trt from src_loss
- removed commented out codes - renamed r2 to r1 - add occupants, removed trt
- remove commented out lines
- remove commented out lines - changed all_indicators to indicators
- remove commented out lines - changed all_indicators to indicators
- changed all_indicators to indicators
- changed all_indicators to indicators
- remove commented out lines - changed all_indicators to indicators
…ste.sql - remove commented out lines - changed all_indicators to indicators
…e.sql - remove commented out lines - changed all_indicators to indicators
…e_ste.sql - remove commented out lines - changed all_indicators to indicators
…e_ste.sql - remove commented out lines - changed all_indicators to indicators
- remove commented out lines - changed all_indicators to indicators
- remove commented out lines - changed all_indicators to indicators - remove sH_RupAbbr
- remove commented out lines - remove sh_rupabbr
- remove commented out lines - changed all_indicators to indicators
- remove commented out lines - changed all_indicators to indicators
- remove commented out lines
- changed all_indicators to indicators - updated VHt_NonResHa
- remove commented out lines - changed r2 to r1
- remove commented out lines - changed all_indicators to indicators - changed r2 to r1
- remove commented out lines - changed all_indicators to indicators - changed r2 to r1
- comment out P/T not ready yet for initial testing
- Et_RESHD to Et_ResHD
- change all_indicators to indicators
update on_uhs to pe_uhs
added code to round indicators to 6 dp
args = parse_args() | ||
|
||
# Copy agg curves stats b0 table | ||
with open("/usr/src/app/ebRisk/Canada/ebR_Canada_agg_curves-stats_b0.csv".format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid system paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass the path in as an argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe get the path from an environment variable, and with a default fallback if the environment variable is unset?
(Or maybe accept both command-line argument and environment variable?)
I took a brief look at it back in June 2021, and from what I recall, it is not just the Python scripts, but also some SQL files where the /usr/src/app
path is hardcoded. See OpenDRR/opendrr-api#114, and the newly opened #98.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. Eventually, it might be worth looking at ways to reduce the reliance on system paths for files if possible. I would like to to see the renovate.json configuration file removed and the requirements.txt updated to reflect current state. I ran some of the python scripts through Flake8 and there are unused imports in many files. Suggest that we enable Flake8 linter in VS Code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkhchow and @drotheram Wow! There are so many intricate details that it is nothing short of amazing how you meticulously keep track of them and make them work! Kudos!
6f2ae3a
to
6710c72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big changes. Everything looks good to me. Note, PSRA results for B.C. are still not included in this pipeline yet
Fixes: