Remove 760m/s boundary on μ formula (G/Gmax param) in HH model by xia-fr · Pull Request #35 · Caltech-geoquake/PySeismoSoil

xia-fr · 2025-10-13T06:48:58Z

We remove the 760m/s Vs limit which was resulting in occasional instabilities for smooth Vs profiles that cross the 760m/s boundary. (correct this time)

jsh9 · 2025-10-13T06:58:55Z

PySeismoSoil/helper_hh_calibration.py

-            mu[j] = 1.0
-        # END IF
+        # softer soil: use Vardanega & Bolton (2011) CGJ formula
+        # fx (251012): we remove the 760m/s limitation on applying the


No need to use name and date in comments. Git blame and git history can keep track of who/when made this change.

Got it, I'll keep that in mind.

Removed the initials and date on the explanatory comment in the code

jsh9 · 2025-10-14T02:36:54Z

Hi @xia-fr , just a reminder that quite a few unit tests failed. You can see the Checks (16) section for more details. These need to be fixed before we can merge changes.

Modified the benchmark in case_2_with_both_Vs_profile_and_GGmax_curves()

Addressing DeprecationWarning: Passing None into shape arguments as an alias for () is deprecated.

jsh9 · 2025-10-15T04:48:34Z

PySeismoSoil/helper_hh_calibration.py

+        # formula, where mu was being set to 1.0 for Vs higher than 760m/s,
+        # which caused occasional instabilities for Vs profiles that cross
+        # 760m/s in a smooth manner. We allow the CGJ formula to apply
+        # to stiffer soils as well.


Hi @xia-fr , I trust you know what you are doing by making these changes.

Can you update versions in pyproject.toml, and also add an entry to CHANGELOG.md?

In your comment here, there's no need to say what you changed (because future code readers can only see the new code in the Python file, not the code diff). Write "what's changed" into CHANGELOG and the PR title (which will become the commit message, which is intended to describe "what's changed in this commit").

jsh9 · 2025-10-15T04:52:37Z

tests/test_helper_generic.py

-        self.assertTrue(
-            np.allclose(hlp.extend_scalar(2.5, None), np.array(2.5))
-        )
+        self.assertTrue(np.allclose(hlp.extend_scalar(2.5, ()), np.array(2.5)))


Instead of replacing an existing test case, you can add your new test case below the old one.

jsh9 · 2025-10-16T03:40:51Z

I'd update the PR title this way:

- Removed 760m/s boundary on mu estimation formula when generating G/Gmax curve parameters for the hybrid hyperbolic model
+ Remove 760m/s boundary on μ formula (G/Gmax param) in HH model

Because commit messages are better to be very concise, so imperative mood and abbreviations are preferred wherever possible.

(VS code even warns people when their commit msg exceeds 50 chars)

jsh9 · 2025-10-16T03:43:10Z

run_linting.sh

@@ -1,2 +1,2 @@
 pip install flake8
-flake8 ./PySeismoSoil --count --statistics --ignore=C901,E203,E741,E221,W605,E502,E116,W504,E266,E114,E222 --max-line-length=99 --max-complexity=10


I think we should delete this file, because linting/workflow is done in tox and Gitlab workflow.

You can leave this file unchanged in this PR and I can deal with it in my other PR.

jsh9 · 2025-10-16T03:44:03Z

pyproject.toml

    "Programming Language :: Python :: 3 :: Only",
 ]
-requires-python = ">=3.9"
+requires-python = ">=3.10"


There's a place in README.md that says "requires python ≥ 3.9". Can you update that place too?

I removed the line that said "3.9" under the section for "Supported Python versions", so it just shows 3.10 to 3.13

jsh9 · 2025-10-16T03:45:42Z

CHANGELOG.md

I guess we should probably update this date too

I edited the date corresponding to 0.6.3 to today's date, lmk if that should be something else

jsh9 · 2025-10-16T03:49:06Z

.pre-commit-config.yaml

@@ -1,32 +1,48 @@
+---


The changes I made in this config may be too disruptive because they'll format other places irrelevant to your current change.

This could pollute the commit with many unrelated changes (which will then pollute Git blame and Git history, making future debugging more time consuming).

You can keep this file as is in this PR, and I'll handle them in my PR.

I think the checks are currently failing here because of something related to the old tox/pytest setup and not because of the actual functionality of the code (as can be seen by them not failing in your PR)

I see. What I meant was: if you change this file in your PR here, you'll introduce hundreds of lines of diffs that are unrelated to your intention (remove 760 m/s boundary in HH model formulas).

Got it, that makes sense.

jsh9 · 2025-10-16T04:17:04Z

Github workflow failed because of 2 reasons:

This error here:

              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  E       TypeError: Use () not None as shape arguments

It relates to this change: #35 (comment)

I guess newer versions of numpy requires () rather than None now. You can change it to ()

Auto formatting.

You can run pre-commit run -a in your terminal before you commit your changes. (Hence then name "pre commit")

jsh9 · 2025-10-16T04:20:42Z

It's becoming more of a convention nowadays that:

A .pre-commit-config.yaml file in a repo means maintainers can run pre-commit run -a to auto-format (or auto-check) code or files
A tox.ini in a repo means tests and checks will be run via tox

jsh9 · 2025-10-17T16:25:20Z

Hi @xia-fr , I think this PR all looks good.

You can click "squash and merge", and draft a new release.

Go to "Releases"

then "draft a new release":

Create a tag v0.6.3:

And click "Generate release notes":

jsh9 · 2025-10-19T17:43:34Z

Hi @xia-fr , the 0.6.3 release somehow showed up as a draft (i.e., not published), so I just published it manually: https://github.com/Caltech-geoquake/PySeismoSoil/releases/tag/v0.6.3

Update helper_hh_calibration.py

a86e60e

We remove the 760m/s Vs limit which was resulting in occasional instabilities for smooth Vs profiles that cross the 760m/s boundary. (correct this time)

jsh9 reviewed Oct 13, 2025

View reviewed changes

Minor comment edit

9667afe

Removed the initials and date on the explanatory comment in the code

xia-fr added 9 commits October 13, 2025 21:20

Adjusting test corresponding to hh calibration

4164ef9

Fixing hh calibration test

95708e6

Modified test file for param from profile

8eb0fb8

Minor formatting edit

4772d42

Update test_class_hh_calibration.py benchmarks

540c5a5

Modified the benchmark in case_2_with_both_Vs_profile_and_GGmax_curves()

Minor formatting edits

73c13fe

Commas formatting

db10016

Update test for deprecation warning

47576d7

Addressing DeprecationWarning: Passing None into shape arguments as an alias for () is deprecated.

Formatting fix

7c7878c

jsh9 reviewed Oct 15, 2025

View reviewed changes

xia-fr changed the title ~~Update helper_hh_calibration.py~~ Removed 760m/s boundary on mu estimation formula in helper_hh_calibration.py Oct 16, 2025

xia-fr changed the title ~~Removed 760m/s boundary on mu estimation formula in helper_hh_calibration.py~~ Removed 760m/s boundary on mu estimation formula when generating G/Gmax curve parameters for the hybrid hyperbolic model Oct 16, 2025

xia-fr added 6 commits October 15, 2025 19:01

Python version and changelog edits

662fe8f

Workflow edit

0807f4f

Reverting test change

b92276d

More workflow changes

60b2cd0

Reverting some changes + pytest adjustment

7eec1ac

Fix testenv

66e592c

jsh9 reviewed Oct 16, 2025

View reviewed changes

xia-fr changed the title ~~Removed 760m/s boundary on mu estimation formula when generating G/Gmax curve parameters for the hybrid hyperbolic model~~ Remove 760m/s boundary on μ formula (G/Gmax param) in HH model Oct 16, 2025

Changelog and readme edits

1dbd3a0

Reverting some changes

d209c02

Formatting and test_helper_generic edit

76e501c

xia-fr merged commit 1455bbd into main Oct 17, 2025
13 checks passed

		@@ -1,2 +1,2 @@
		pip install flake8
		flake8 ./PySeismoSoil --count --statistics --ignore=C901,E203,E741,E221,W605,E502,E116,W504,E266,E114,E222 --max-line-length=99 --max-complexity=10

Comments

Conversation

xia-fr commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsh9 commented Oct 14, 2025

Uh oh!

jsh9 Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsh9 commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsh9 Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsh9 commented Oct 16, 2025

Uh oh!

jsh9 commented Oct 16, 2025

Uh oh!

jsh9 commented Oct 17, 2025

Uh oh!

Uh oh!

jsh9 commented Oct 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jsh9 Oct 15, 2025 •

edited

Loading

jsh9 commented Oct 16, 2025 •

edited

Loading

jsh9 Oct 16, 2025 •

edited

Loading