-
Notifications
You must be signed in to change notification settings - Fork 305
Add Mohammadi2023Turkiye ML-based GSIM #11006
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
base: master
Are you sure you want to change the base?
Conversation
|
Dear Amir, Many thanks for providing an implementation of your machine-learning based GSIM. I will do my best to review this model by the end of the month. One thing I notice immediately, is that there are far too many Perhaps @micheles can give some input on this too. Thanks, Christopher |
Comment out Slack notification steps in failure workflow.
|
Dear Christopher (@CB-quakemodel), Happy New Year, and I apologise for the raised issue. Bests |
Dear @amirxdbx, Thanks for making the changes, and a happy new year to yourself also. I see many ONNX files are now included in a different location and were not included in the initial commits you made earlier today, so perhaps these are unintentionally remaining in your PR? Also, the workflow_failure.yaml has been modified in your PR, so you should revert this file given it is likely unintentionally modified here. Thanks, Christopher |
|
Dear Christopher, |
Many thanks! I will complete my review early this week. |
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.
Thanks again @amirxdbx, I have made an initial review with only a handful of comments/requested changes (nearly all of them are just to make things cleaner/easier to digest as an external user reviewing the GSIM module). The only moderately big change is to ensure safe handling of any external files used in the GSIM (please see my comments within the review).
Please note that the docs tests are failing primarily due to the formatting changes I suggest in my review - please see here for more info on the required formatting changes too.
|
Thanks for the review, Christopher! I’ve updated the code to address your comments. I moved the documentation into the class strings and added the init method to handle the CSV loading safely as you suggested. I also cleaned up the global variables, fixed the spacing, and consolidated those checks. Let me know if everything looks good now! |
…ompliance for CI docs
|
Dear @CB-quakemodel, Thanks again for guiding me on the path. I have addressed your previous comments. I hope this fixes all the fails. Bests |
This PR adds a new machine-learning-based Ground Motion Model for Turkey
(Mohammadi et al., 2023) implemented as a GSIM in the OpenQuake hazard library.
Key features:
• Implements the vectorized compute() API (no legacy methods).
• Wraps the official XGBoost models exported to ONNX runtime.
• Includes standard deviations (sigma, tau, phi) from stds.csv.
• Includes verification tables and regression tests.
• Documentation updated in hazardlib.gsim.rst.
• Changelog updated.
The GSIM-specific tests pass successfully.