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
taup: added a time plot -- analogue to the existing ray path plot #1877
Conversation
@kaspervanwijk Can you post an example image? |
obspy/taup/tau.py
Outdated
|
||
:returns: The (possibly created) axes instance. | ||
:rtype: :class:`matplotlib.axes.Axes` | ||
|
||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reformat docstring to (to be in line with rest of codebase):
"""
Method to plot the travel times of arrivals class, if any have
been calculated.
:param ax: Axes to plot to. If not given, a new figure with an axes
will be created. Must be a polar axes for the spherical plot and
a regular one for the Cartesian plot.
:type ax: :class:`matplotlib.axes.Axes`
:param show: Show the plot.
:type show: bool
:returns: The (possibly created) axes instance.
:rtype: :class:`matplotlib.axes.Axes`
"""
Oh and phases
is missing in docstring..
Ah OK, so this is basically a replacement for the old plot that is not working in master anymore? https://docs.obspy.org/tutorial/code_snippets/travel_time.html#travel-time-plot |
That's right. A replacement (hopefully; this is my first attempt at a PR), but the same way the ray paths are currently plotted: as a method on "arrivals." |
This will be a really nice PR! Thanks already! Having this as a method on arrivals is very nice API-wise, but I think also a convenience wrapper function for the whole process would also be a real handy addition. Something like this.. def traveltime_plot(source_depth, phases=['P', 'S'], model='iasp91',
distance_range=(0, 180)):
... I'll add some todos in the OP. Let us know if you need help setting up the image test or adding the plot to the gallery. |
…ch the convention in obspy.
For distances over 180 degrees, this could optionally be done in the plot doing a wraparound like here: in "Jeffreys/Bullen style" but it might be a bit tricky to get the duplicated x-axis labels right.. |
Could make the wrapper function you mention, but that would be very similar to the old way (except it would not call deprecated code, but properly use TauPyModel.get_travel_times(). Would you want both the method and the wrapper function? As is, the plot handles >180 correctly, I believe. (Yes, I will need some tips on how to do an image test and gallery add) |
Yes, I would implement both. For the user it's much more convenient to just have a single function call rather than use the above code snippet. But having the logic as a method with |
Requesting a docs build for this PR, so that we can check the example plot: +DOCS |
obspy/taup/tau.py
Outdated
|
||
|
||
def traveltime_plot(min_degree=0, max_degree=360, npoints=1000, | ||
phases=['P', 'S'], source_depth=10, model='iasp91', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for making source_depth
an argument, not a kwarg
with default value, such that people have to explicitly set their source depth of choice. Otherwise people might overlook it's importance for the output and I feel like there's no generally useful default value for it.
obspy/taup/tau.py
Outdated
|
||
:param min_degree: minimum distance from the source (in degrees) to | ||
plot travel times Defaults to ``0``. | ||
:type min_degree: float, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we nowadays usually leave away the , optional
part (I know we still have it in older parts of the docs).
It's just plain redundant to add it for just every kwarg
out there and only clutters the docs, IMHO.
obspy/taup/tau.py
Outdated
:param source_depth: Source pepth in kilometers. Defaults to ``10``. | ||
:type source_depth: float, optional | ||
:param model: string containing the model to use. | ||
:type model: str. Defaults to 'iasp91' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value should be mentioned in the :param ..:
section below, I think, rather than the :type :
section
obspy/taup/tau.py
Outdated
|
||
.. rubric:: Example | ||
|
||
>>> from obspy.taup.tau import traveltime_plot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plot will be very useful, I'd vote to add it's import to obspy/taup/__init__.py
so that it can be imported as from obspy.taup import traveltime_plot
. Accordingly then all examples should also use that shorter import notation.
This is looking real nice already. :-) To add it to the gallery, just replace the old example that's still in there (broken of course). I.e. adapt For the image test, just copy/paste/adapt one of the test routines in |
Btw.. If you want to see what flake8 thinks of your commits right when doing them, you could set up a local git hook, see first paragraph on this page: https://docs.obspy.org/master/coding_style.html#obspy-coding-style-guide It's really useful. |
Docs build for the PR is up (will be automatically updated on new commits): http://docs.obspy.org/pull-requests/1877/packages/obspy.taup.html#module-obspy.taup Example plot's not working..: http://docs.obspy.org/pull-requests/1877/packages/autogen/obspy.taup.tau.traveltime_plot.html#obspy.taup.tau.traveltime_plot See http://docs.obspy.org/pull-requests/1877/log.txt, searching for
Can you also add a short paragraph with plot to the taup landing page (i.e. |
@kaspervanwijk I just pushed some changes, would be good if you breifly check them. I'll start rebasing intermediate images out of this now and will then force-push over this branch. For reference, current tip is at 0a0b8ac. |
thank you, @megies, your edits are all improvements. I agree with making the default number of travel times to calculate a smaller number to speed up the code, but hope we can keep a nice dense graph in the gallery. |
fair enough, i'll crank it up |
plot type (Spherical/cartesian)
this can be used in tests that should fail and then might attempt to upload images which takes several seconds and is totally unwanted in this case
…mits also sanitize ax/fig kwarg handling in one routine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be merged. Travis is failing virtually every image test right now, and since this PR is coming from a fork the imgur uploads from Travis don't work unfortunately, because the imgur token in the secure env variable is not available for PRs from forks.
Anyway, good to merge, IMHO.
Thanks a bunch @kaspervanwijk, great work on this PR! |
see 9bd3a80 |
What does this PR do?
create a method on the arrivals object for a travel time plot, analogue to the ray path plot method. It replaces the old (and removed
travelTimePlot
function, see https://docs.obspy.org/archive/1.0.3/tutorial/code_snippets/travel_time.html#travel-time-plot)Why was it initiated? Any relevant Issues?
Fixes #1501, fixes #1330
PR Checklist
CHANGELOG.txt
.CONTRIBUTORS.txt
.Todos