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

Wiggly lines for S waves in plot ray paths (addresses issue #3042) #3047

Merged
merged 12 commits into from Apr 13, 2022
Merged

Wiggly lines for S waves in plot ray paths (addresses issue #3042) #3047

merged 12 commits into from Apr 13, 2022

Conversation

johnrudge
Copy link
Member

@johnrudge johnrudge commented Apr 12, 2022

What does this PR do?

This PR adds a flag indicate_wave_type (bool) to the Arrivals.plot_rays() method. When True the S waves are plotted as wiggly lines.

Why was it initiated? Any relevant Issues?

This fixes #3042. The PR adds a function split_ray_path to taup.utils which is needed to split the path at discontinuities so that the P and S waves can be plotted with separate line styles. The split_ray_path function figures out whether the waves are P or S by looking at their delay time. There may well be more elegant ways of splitting the ray path, but the current method seems robust.

As for tests, I'm not quite sure how to test plotting features, so I've not added a test yet.

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.

Code by myself and Stuart Russell, so I've added both our names to the CONTRIBUTORS.txt.
+DOCS

obspy/taup/tau.py Outdated Show resolved Hide resolved
@megies megies added build_docs Docs will be automatically built and deployed in github actions on pushes to the PR .taup .imaging Issues with our plotting functionalities upload_images Signals CI to attach all produced images as github artifacts. labels Apr 12, 2022
@megies megies added this to the 1.4.0 milestone Apr 12, 2022
@megies
Copy link
Member

megies commented Apr 12, 2022

Code by myself and Stuart Russell, so I've added both our names to the CONTRIBUTORS.txt.

👍 totally fine =)

@megies
Copy link
Member

megies commented Apr 12, 2022

As for tests, I'm not quite sure how to test plotting features, so I've not added a test yet.

Image comparison tests arent run currently anyway, it's on our TODO to bring those back. We could simply have one of the plots in the Tutorial use this new option.

Maybe this one: https://docs.obspy.org/master/tutorial/code_snippets/travel_time.html#spherical-ray-paths

Or this last one on the page: https://docs.obspy.org/master/tutorial/code_snippets/travel_time.html#ray-paths-for-multiple-distances

Or maybe also on the taup front page: https://docs.obspy.org/master/packages/obspy.taup.html?highlight=taup#plotting

(see here to change it in Tutorial and here for the taup API front page)

@megies
Copy link
Member

megies commented Apr 12, 2022

Ideal would be if you rebase this on top of current master, to get rid of the merge commit and the duplication of changes. But, you'd have to fix some conflicts in the rebase manually with indentation changes involved and all, so if it's too much to ask, then it's fine too.

@megies
Copy link
Member

megies commented Apr 12, 2022

On the next push to this branch you should also get the docs built and deployed (takes about 20mins), and plots in the docs can be checked here: https://docs.obspy.org/pr/

@johnrudge
Copy link
Member Author

I've moved the kwarg to the end, and I've added the flag to the final tutorial example travel_time_body_waves.py as it has a nice mix of P and S waves.

Figure_1

I'm afraid I was struggling with rebasing, so I've left as is. How do I get the docs rebuilt?

@megies
Copy link
Member

megies commented Apr 12, 2022

I'm afraid I was struggling with rebasing, so I've left as is.

Totally fine, like I said, happy to have such quality contributions without much work left to do, and at the end of the day it's just git history cosmetics, so don't worry. =)
Most of the time, rebasing is quite trivial, but whenever it involves something with indentation changes (like these color things moved left one level) is where the diff gets ugly and you basically have to redo all the code manually, as git diff can handle anything on the same indentation level neatly, but not so much if it changes.

How do I get the docs rebuilt?

Any commit pushed to a pull request or one of our main branches directly will trigger continuous integration and our github actions (what we use for CI these days) is set up so that any PR with the "build_docs" label attached will build docs and they will get made accessible on our server at https://docs.obspy.org/pr/

I don't know exactly what's goin on, earlier I hit this button "workflows await approval" > "approve and run" so I thought all is in line (you can see github ran CI for an earlier commit), but now I get this shown to me again.. CC @trichter

In any case, I'll just add you to obspy dev team, that should get rid of that thing, and you can also push branches for PRs to the main repo if you prefer (which can make it easier for other people to chime in with commits)


Screenshot from 2022-04-12 18-30-19


Screenshot from 2022-04-12 18-31-21

@trichter
Copy link
Member

I don't know exactly what's goin on, earlier I hit this button "workflows await approval" > "approve and run" so I thought all is in line (you can see github ran CI for an earlier commit), but now I get this shown to me again.. CC @trichter

Don't know either. Maybe its because, now another workflow (the docs building) needs to be started. Usually workflows need to be approved only once.

@megies
Copy link
Member

megies commented Apr 12, 2022

Don't know either. Maybe its because, now another workflow (the docs building) needs to be started. Usually workflows need to be approved only once.

Could be. Let's just ignore it for now, if we hit it again we can worry about it =)

@johnrudge
Copy link
Member Author

Thanks both! It says all check passed, but I can't see the documentation at https://docs.obspy.org/pr/
I locally built the docs on my machine and they look fine though. Next steps?

@trichter
Copy link
Member

trichter commented Apr 12, 2022

Docs deployment is working again. It was not working since 4 days, because the value of the status field in the answer to the following request changed from "success" to "completed". I adapted the server-side script.

curl -H "Accept: application/vnd.github.v3+json" https://api.github.com/repos/obspy/obspy/actions/runs?per_page=1
{
  "total_count": 1844,
  "workflow_runs": [
    {
      "id": 2155599922,
      "name": "docs",
      "node_id": "WFR_kwLOAFd0FM6Ae9gy",
      "head_branch": "improve-plot-ray-paths",
      "head_sha": "86de2de47fd138f703148965bf93c807decd9ffc",
      "run_number": 507,
      "event": "pull_request",
      "status": "completed",
      "conclusion": "success",

...

trichter added a commit that referenced this pull request Apr 12, 2022
@johnrudge johnrudge requested a review from megies April 13, 2022 07:05
@megies
Copy link
Member

megies commented Apr 13, 2022

Next steps?

Nothing much, I'll give it a final glance in a bit, then merge. =)

https://docs.obspy.org/pr/improve-plot-ray-paths/tutorial/code_snippets/travel_time.html#spherical-ray-paths

Looking beautiful in docs!

https://docs.obspy.org/pr/improve-plot-ray-paths/tutorial/code_snippets/travel_time_body_waves.png

travel_time_body_waves

the value of the status field in the answer to the following request changed from "success" to "completed".

Euh! Thanks for finding and fixing it =)

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.

Awesome stuff, great job on this PR! ❤️

@megies megies merged commit 825401d into obspy:master Apr 13, 2022
@johnrudge
Copy link
Member Author

Thanks for reviewing and merging it so quickly!

@johnrudge johnrudge deleted the improve-plot-ray-paths branch April 13, 2022 14:58
@QuLogic
Copy link
Member

QuLogic commented Apr 14, 2022

I don't know exactly what's goin on, earlier I hit this button "workflows await approval" > "approve and run" so I thought all is in line (you can see github ran CI for an earlier commit), but now I get this shown to me again.. CC @trichter

Don't know either. Maybe its because, now another workflow (the docs building) needs to be started. Usually workflows need to be approved only once.

Workflows for new contributors must be approved every time, i.e., for every new push.

@jrekoske
Copy link
Contributor

@johnrudge Thanks for getting to my issue so quickly! This is a really neat addition to obspy.

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 .imaging Issues with our plotting functionalities .taup upload_images Signals CI to attach all produced images as github artifacts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different plotting style for P- and S-waves
5 participants