New GSIM Idini2017 #6666
New GSIM Idini2017 #6666
Conversation
pheresi
commented
Apr 2, 2021
pheresi
commented
- Implements Idini et al (2017) gsim, including its test and verification tables
- Includes soiltype into site.py and adds it in site_test.py and oqvalidation.py
Implements Idini et al (2017), its test and test tables Includes soiltype into site.py and adds it in site_test.py and oqvalidation.py
|
Since you are extending the site model we also need an end-to-end test using a site model with |
|
Oh ok, perfect. Should I create a new case (case_38) from case_37, and include it in this pull request, or create another one? |
|
Oops... just saw that there is already a case_38... So well, I mean case_63 |
|
You can create a new test case or update an existing one to use |
|
You are probably doing more work than needed. Adding |
|
Sorry, sorry... yesterday I couldn't work on this, but I'll do it today. |
|
There are some conflicts to resolve, for the rest LGTM |
|
Oh, right. I'm going to resolve those in a bit. Thanks for the reminder! |
|
Hi again, |
openquake/hazardlib/site.py
Outdated
| @@ -252,6 +253,7 @@ def from_points(cls, lons, lats, depths=None, sitemodel=None, | |||
| self._set('z2pt5', sitemodel.reference_depth_to_2pt5km_per_sec) | |||
| self._set('siteclass', sitemodel.reference_siteclass) | |||
| self._set('backarc', sitemodel.reference_backarc) | |||
| self._set('soiltype', sitemodel.reference_soiltype) | |||
There was a problem hiding this comment.
Please remove this line and the test should become green again
There was a problem hiding this comment.
But then how can we define a reference soiltype for all the sites in a region?
There was a problem hiding this comment.
Just write a site model with soiltype constant for all sites. If you look at the PR that I mentioned before (https://github.com/gem/oq-engine/pull/6672/files) the "bas" term is introduced only as a local site parameter. Not introducing reference_soiltype not only makes your life easier, it also avoids building a site collection larger than needed for all models not using Idini2017, i.e. all model in existence before this PR ;-)
It was a mistake to introduce the siteclass as a global parameter, I should remove that one too.
There was a problem hiding this comment.
oh, you are totally right! let me remove the line update
Deleting reference_soiltype
|
You forgot to commit case_63/expected/hcurve-mean.csv |
|
Yep, was a mistake with the file names... updating! :) |
|
Umm... Im not sure why the classical_test failed in my case_63... apparently, the number of lines in the hazard curve files are different |
|
It looks looks like a wrong newline ending. You must remove a ^M character: @@ -1,3 +1,3 @@
-#,,,,,,"generated_by='OpenQuake engine 3.12.0', start_date='2021-04-07T11:23:52', checksum=2265841407, kind='mean', investigation_time=50.0, imt='PGA'"
-lon,lat,depth,poe-0.0100000,poe-0.1000000,poe-0.2000000,poe-0.5000000
-172.63000,-43.53000,0.00000,5.934701E-03,5.458004E-03,3.789143E-03,9.009379E-04^M
+#,,,,,,"generated_by='OpenQuake engine 3.12.0-gitd11f73014f', start_date='2021-04-09T06:16:57', checksum=2265841407, kind='mean', investigation_time=50.0, imt='PGA'"
+lon,lat,depth,poe-0.0100000,poe-0.1000000,poe-0.2000000,poe-0.5000000
+172.63000,-43.53000,0.00000,5.934701E-03,5.458004E-03,3.789143E-03,9.009379E-04And now there are conflicts again, sorry ;-) |
|
Got it! Updating :) |
…into GSIM_Idini2017
|
Finally! :) Michele, thanks for your help, and sorry for the spam! |
