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

Add support for inner core diffracted phases in taup with Kdiff #3095

Merged
merged 9 commits into from Jun 22, 2022

Conversation

johnrudge
Copy link
Member

@johnrudge johnrudge commented Jun 20, 2022

What does this PR do?

This pull request adds support for inner core diffracted phases in obspy.taup with the notation Kdiff. An example is:

from obspy.taup import TauPyModel
model = TauPyModel("ak135")
arrivals = model.get_ray_paths(0.0, 160, ["PKdiffP"])
print(arrivals)
arrivals.plot_rays()

Figure_1

2 arrivals
        PKdiffP phase arrival at 1213.904 seconds
        PKdiffP phase arrival at 1296.514 seconds

A very small tweak is also made to the plot_travel_times function updated in #3092. It better plots the wrap-around phases at 180 and 360 degrees by plotting with separate lines. Compare the far right of the new plot
plot_travel_times
with the old one of #3092.

Why was it initiated? Any relevant Issues?

Support for inner core diffracted phases has been requested for the java version of taup, see crotwell/TauP#3 . This PR supports the PKP_Cdiff phase requested in that issue, but not the PKP_Bdiff phase. @crotwell -- can you take a look and see if you want to add Kdiff to the java version too?

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 add the "build_docs" tag to this PR.
    Docs will be served at docs.obspy.org/pr/{branch_name} (do not use master branch).
    Please post a link to the relevant piece of documentation.
  • If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the "test_network" tag to this PR.
  • All tests still pass.
  • Any new features or fixed regressions are covered via new tests.
  • Any new or changed features are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts.
  • Add the "ready for review" tag when you are ready for the PR to be reviewed.

@johnrudge johnrudge added .taup build_docs Docs will be automatically built and deployed in github actions on pushes to the PR labels Jun 20, 2022
@johnrudge johnrudge requested a review from megies June 20, 2022 09:32
@johnrudge
Copy link
Member Author

Kdiff has been noted in the docs: https://docs.obspy.org/pr/inner_core_diffraction/packages/obspy.taup.html#module-obspy.taup in the Phase naming section. Over to you @megies to review.

@megies megies added this to the 1.4.0 milestone Jun 21, 2022
self.min_ray_param):
if (self.current_branch < tau_model.iocb_branch - 1 or
(self.current_branch == tau_model.iocb_branch - 1
and end_action != _ACTIONS["diffract"])):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, I think in all these cases my comment in the other ticket applies in the same way. Readability could be much improved by using named variables for the individual parts of the conditions. But I don't want to hassle you too much with it, if you feel it's OK to do it this way (I assume it might be like this throughout the taup code base..?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the readibility could be improved, but I'm afraid I'm not inclined to do so for this PR. What I've done here done is consistent with the existing style (e.g. see how Pdiff and Sdiff are treated in lines 555-604). Really there should be some refactoring and cleaning up of the whole complicated SeismicPhase.parse_name method, and this should be done consistently e.g. whatever is done for Kdiff should also be done for Pdiff. But I think that's a big job, and requires careful thought.

I'm fine with the cosmetic change you made.

Copy link
Member

@megies megies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one cosmetics comment, just let me know if you wanna keep as is, thats fine with me too I guess

@megies megies merged commit 2ed3225 into master Jun 22, 2022
@megies megies deleted the inner_core_diffraction branch June 22, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_docs Docs will be automatically built and deployed in github actions on pushes to the PR .taup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants