-
Notifications
You must be signed in to change notification settings - Fork 279
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
Improving extensibility of NGA-West2 and NGA-Sub ground-motion model implementations #10011
Comments
There is nothing forbidding passing the CoeffsTable at instantiation time, i.e. it is possible to override a default table with code like the following: from openquake.hazardlib import const
from openquake.hazardlib.gsim.base import GMPE
from openquake.hazardlib.imt import PGA, SA
from openquake.hazardlib.gsim.coeffs_table import CoeffsTable
class Example(GMPE):
DEFINED_FOR_TECTONIC_REGION_TYPE = const.TRT.STABLE_CONTINENTAL
DEFINED_FOR_INTENSITY_MEASURE_TYPES = {PGA, SA}
DEFINED_FOR_INTENSITY_MEASURE_COMPONENT = const.IMC.GEOMETRIC_MEAN
DEFINED_FOR_STANDARD_DEVIATION_TYPES = {const.StdDev.TOTAL}
REQUIRES_SITES_PARAMETERS = set()
REQUIRES_RUPTURE_PARAMETERS = {'mag'}
REQUIRES_DISTANCES = {'rjb'}
COEFFS = CoeffsTable(sa_damping=5, table="""\
imt a1 a2 a3
PGA 0.1 0.2 0.3
0.01 0.4 0.5 0.6
0.05 0.7 0.8 0.9""")
def __init__(self, coeffs=None):
if coeffs is not None:
self.COEFFS = coeffs
if __name__ == '__main__':
region_coeffs = CoeffsTable("""
imt a1 a2 a3
PGA 0.11 0.22 0.33
0.01 0.43 0.54 0.66
0.05 0.7 0.8 0.9""")
example_original = Example() # default COEFFS
example_regional = Example(coeffs=region_coeffs) # overridden COEFFS It is just a matter of adding a few aliases and introducing a TOML representation for the CoeffsTable. |
For a region-specific ground-motion model that needs to override many of the coefficients, your approach works well. This is what I am currently doing. In my case, I am only changing 2-3 out of 20+ coefficients; for the NGA-West2 and NGA-Sub ground-motion models, there are generally only 2-3 region-specific coefficients but 20+ coefficients that are not region-specific. I would rather override just the region-specific coefficients rather than 20+ coefficients when most of the coefficients don't change. This seems like a much cleaner implementation. Additionally, overriding all of the coefficients it makes it difficult for a user of the new region-specific ground-motion model to understand what coefficients were actually changed. |
What about def __init__(self, coeffs_toml=''):
if coeffs_toml:
self.COEFFS = self.COEFFS | CoeffsTable.fromtoml(coeffs_toml) where CoeffsTable.fromtoml('''
["SA(0.01)"]
a1 = 0.11
''') I am working on this idea to refactor the CampbellBozorgnia GMPEs, see #10024. |
This looks like an excellent solution! |
I got the CampbellBozorgnia classes working with a few minor tweaks and you can look at those for inspiration (see https://github.com/gem/oq-engine/blob/master/openquake/hazardlib/gsim/campbell_bozorgnia_2014.py#L412) |
Shortcoming of current implementation
For the 2025 USGS NSHM update for Puerto Rico we are creating local (regional) adjustment to the NGA-West2 (
AbrahamsonEtAl2014
,BooreEtAl2014
,CampbellBozorgnia2014
, andChiouYoungs2014
) and NGA-Sub (AbrahamsonGulerce2020
,KuehnEtAl2020
, andParkerEtAl2020
) ground-motion models. The existing implementations in OQ for the NGA-Sub models account for regional variations with regions hardwired in the code and all of the region-specific coeffients included in a single coefficients table. As a result , it is difficult to extend the implementation to other regions without modifying the existing files. Similar issues will likely come up more frequently as the community moves towards non-ergodic models.Potential solution
We are interested in contributing this change in the coming months. We would appreciate comments on whether this general direction makes sense or if you have an alternative approach in mind for making the ground-motion model implementations more extensible.
This is somewhat related to #9894. The
ModifiableGMPE
approach seems limited because the class includes logic related to the modifications. I strongly prefer that the extension be completely self-contained.The text was updated successfully, but these errors were encountered: