Skip to content
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

build_taup_model() fails for constant-velocity models with more than 2 layers #2264

Merged
merged 3 commits into from Feb 15, 2019

Conversation

Projects
3 participants
@nackerley
Copy link
Contributor

nackerley commented Nov 15, 2018

A layered constant-velocity model can have more first-order discontinuities than a piecewise-linear model. The number of first-order discontinuities in a constant-velocity model is number of layers + 1. Since the minimum number of layers is 1, this should never be less than 2. [n.b. that other issues prevent construction of a 1-layer model, but who cares?]

Here is where the storage for discontinuities is set up:

self.critical_depths = np.empty(
max(len(self.v_mod.layers), 3),
dtype=CriticalDepth)

As you can see, the maximum number was the number of layers and the minimum number was 3. This meant that this worked for constant-velocity models with 2 layers, but no more.

I don't understand why numpy.empty() was used; There's no need for speed. Instead I'm proposing numpy.zeros().

The test is a variant on:

def test_small_regional_model(self):
"""
Tests a small regional model as this used to not work.
"""
with TemporaryWorkingDirectory():
folder = os.path.abspath(os.curdir)
model_name = "regional_model"
build_taup_model(
filename=os.path.join(DATA, os.path.pardir,
model_name + ".tvel"),
output_folder=folder, verbose=False)
m = TauPyModel(os.path.join(folder, model_name + ".npz"))
arr = m.get_ray_paths(source_depth_in_km=18.0, distance_in_degree=1.0)
self.assertEqual(len(arr), 9)
for a, d in zip(arr, [("p", 18.143), ("Pn", 19.202), ("PcP", 19.884),
("sP", 22.054), ("ScP", 23.029), ("PcS", 26.410),
("s", 31.509), ("Sn", 33.395), ("ScS", 34.533)]):
self.assertEqual(a.name, d[0])
self.assertAlmostEqual(a.time, d[1], 3)

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

@megies megies added this to the 1.2.0 milestone Nov 16, 2018

@megies megies added the .taup label Nov 16, 2018

@megies

This comment has been minimized.

Copy link
Member

megies commented Nov 16, 2018

Looks reasonable to me, not sure if reducing the minimum length from 3 to 2 could harm anybody though? I guess the minimum was three for a reason, but maybe not.. ;-)

(This might even qualify as a bug fix, for 1.1.1?)

@megies megies requested a review from krischer Nov 16, 2018

nackerley pushed a commit to nackerley/obspy that referenced this pull request Nov 16, 2018

@nackerley

This comment has been minimized.

Copy link
Contributor Author

nackerley commented Nov 16, 2018

I've added tests - I've only checked that the taup test suite has no errors (3 tests skipped).

(This might even qualify as a bug fix, for 1.1.1?)

I think this should be classed as a bug fix - does that mean I need to rebase against a different branch?

@nackerley

This comment has been minimized.

Copy link
Contributor Author

nackerley commented Nov 16, 2018

I guess the minimum was three for a reason, but maybe not.

I think it was a mistaken attempt to fix the problem that I've identified - it meant that build_taup_model() started working for models with 2 layers, but no other models. Single-layer models fail for a whole bunch of reasons, starting with, but not limited to, not being able to find the moho. Larger models because the maximum possible number of discontinuities is the number of layer plus one, and this line of code was missing the plus one. With the "plus one" fix, the minimum value can be removed.

@megies

This comment has been minimized.

Copy link
Member

megies commented Nov 19, 2018

I think this should be classed as a bug fix - does that mean I need to rebase against a different branch?

Yep, in that case you should rebase on maintenance_1.1.x.

@megies megies modified the milestones: 1.2.0, 1.1.1 Nov 19, 2018

@krischer krischer added this to Free for the taking in Release 1.1.1 Feb 14, 2019

@krischer krischer force-pushed the nackerley:feature/fix_taup_1dcvl branch from e1e1e31 to 34764ed Feb 14, 2019

krischer added a commit to nackerley/obspy that referenced this pull request Feb 14, 2019

@krischer krischer changed the base branch from master to maintenance_1.1.x Feb 14, 2019

@krischer krischer force-pushed the nackerley:feature/fix_taup_1dcvl branch from 34764ed to 30307bf Feb 14, 2019

krischer added a commit to nackerley/obspy that referenced this pull request Feb 14, 2019

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Feb 14, 2019

I rebased the commits on the latest maintenance branch so we can get this into 1.1.1. If CI passes this can IMHO be merged.

@krischer krischer moved this from Free for the taking to Waiting for CI in Release 1.1.1 Feb 14, 2019

Nick Ackerley added some commits Nov 15, 2018

For a 1D constant-velocity model, the number of first-order discontin…
…uities is the number of layers + 1. Since the minimum number of layers is 1, this should never be less than 2. I don't understand why "empty" was used here; there's no need for speed.

@megies megies force-pushed the nackerley:feature/fix_taup_1dcvl branch from 30307bf to b4dfb24 Feb 14, 2019

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 14, 2019

Rebased on current master and force-pushed so that we have fresh CI results tomorrow.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Feb 15, 2019

The 1 failure on appveyor on Python 3.7 is unrelated and we've encountered it before.

@krischer krischer merged commit 84e0fc7 into obspy:maintenance_1.1.x Feb 15, 2019

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krischer krischer moved this from Waiting for CI to Done in Release 1.1.1 Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.