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

Velocity reduction units behave like km/s, not m/s #3371

Open
1 task done
liamtoney opened this issue Nov 16, 2023 · 3 comments
Open
1 task done

Velocity reduction units behave like km/s, not m/s #3371

liamtoney opened this issue Nov 16, 2023 · 3 comments
Labels
bug-unconfirmed reported bug that still needs to be confirmed .imaging Issues with our plotting functionalities

Comments

@liamtoney
Copy link
Contributor

Avoid duplicates

  • I searched existing issues

Bug Summary

When using Stream.plot(type='section'), the vred argument for velocity reduction is supposed to be in units of m/s per the documentation. However, providing e.g. vred=0.6 produces effectively a 600 m/s reduction. I.e., there's a m-to-km conversion issue.

The example code below shows that when e.g. vred=1 (1 m/s) is used, the effective velocity reduction is actually 1 km/s — see the red line. Example code's output plot:

20231116001017_fig1

This bug is subtle because for most users, providing a value of 3,000 m/s for vred actually produces velocity reduction of 3,000,000 m/s — which has little time-shifting effect! So nothing immediately looks wrong in the plot.

I believe the issue lies in the fact that WaveformPlotting.__sect_init_time(), which forms the trace time vectors including velocity reduction, uses WaveformPlotting._tr_offsets. But WaveformPlotting._tr_offsets are in km, not in meters. Hence things working as expected when I treat vred as having units of km/s.

I'm happy to make a PR to fix this if the issue I've raised here is clear to the devs. Cheers!

Code to Reproduce

from obspy import read
import matplotlib.pyplot as plt

# Read in example data
st = read()

# Pretend that the three components are actually traces
# from different stations at 10, 20, 30 km from source
for tr, distance in zip(st, [10 * 1000, 20 * 1000, 30 * 1000]):
    tr.stats.distance = distance  # [m]

# Plot record section
vred = 1  # [m/s] Per documentation
fig = plt.figure()
st.plot(fig=fig, type='section', orientation='horizontal', vred=vred)
ax = fig.axes[0]
ymax = ax.get_ylim()[1]  # [km]
fig.axes[0].plot([0, -ymax / vred], [0, ymax], color='red', label=f'{vred} km/s')  # Pretending vred is in km/s here – has to be since y-axis is in km!
ax.legend()
ax.set_title(f'vred = {vred} m/s')
fig.show()

Error Traceback

No response

ObsPy Version?

1.4.0

Operating System?

macOS (M1)

Python Version?

3.11.5

Installation Method?

conda

@liamtoney liamtoney added the bug-unconfirmed reported bug that still needs to be confirmed label Nov 16, 2023
@liamtoney
Copy link
Contributor Author

I'll note that the reason why the moveout is going backwards in time in the example plot is because the traces are aligned to start with (since they're actually just different components of the same station). In case that was confusing!

@megies
Copy link
Member

megies commented Nov 16, 2023

Without looking at details (explanation looks sound), there seems to be two ways to fix this:
a) adjust docs
b) adjust code

a) has the advantage that results wouldn't change for people, assuming they worked around this issue themselves, but would have the disadvantage that if people used this wrong before and do not note the change, they might use it wrong in the future too?
b) results will change for people.. hopefully to the better but could also be to the worse if they worked around the issue before

🤔

@megies megies added the .imaging Issues with our plotting functionalities label Nov 16, 2023
@liamtoney
Copy link
Contributor Author

I had the same thought about the two options. If we do (a) — change the docs to say "km/s" note that we still need to modify

self.sect_vred = kilometer2degrees(self.sect_vred / 1e3)

to remove the conversion from m/s to km/s since vred will already be in km/s.

I don't know which is the better call in (a) or (b) here! Maybe I lean to (a) since if folks don't see expected behavior (i.e. they were using it wrong by providing vred=600 expecting 600 m/s reduction) they will hopefully go back to the docstring and see "oh, it's in km/s!" and then change their code to vred=0.6.

I guess you can never count on people to read the docs tho ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-unconfirmed reported bug that still needs to be confirmed .imaging Issues with our plotting functionalities
Projects
None yet
Development

No branches or pull requests

2 participants