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

Respect method='full' for Stream.plot(type='section') #3379

Merged
merged 5 commits into from Mar 7, 2024

Conversation

liamtoney
Copy link
Contributor

@liamtoney liamtoney commented Dec 1, 2023

What does this PR do?

For section-type plots: Ensures that the full Trace is plotted when a user requests it to be (i.e., when specifying method='full'). Parallels the logic implemented in the WaveformPlotting.plot() method.

Why was it initiated? Any relevant Issues?

Fixes #3375.

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).
  • While the PR is still work-in-progress, the no_ci label can be added to skip CI builds
  • 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.
  • New modules, add the module to CODEOWNERS with your github handle
  • Add the yellow ready for review label when you are ready for the PR to be reviewed.

Parallels the logic implemented in the `WaveformPlotting.plot()` method
@liamtoney
Copy link
Contributor Author

Demo:

from obspy import read
import matplotlib.pyplot as plt
import numpy as np

# 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]

# Artifically increase the number of points in each trace to exceed max_npts
# (this makes the resampling for 'fast' clear)
for tr in st:
    tr.data = np.tile(tr.data, 20)

# Plot record section
fig = plt.figure()
st.plot(fig=fig, type='section', orientation='horizontal', method='fast', alpha=1, color='red')
st.plot(fig=fig, type='section', orientation='horizontal', method='full', alpha=1)
fig.axes[0].set_xlim(5, 10)  # Zoom in to show effect of the different plotting methods
fig.show()

out

@liamtoney liamtoney marked this pull request as ready for review December 1, 2023 01:16
@filefolder
Copy link
Contributor

seems like a nice fix! i think you may have to rebase to the current maintenance branch though

@liamtoney
Copy link
Contributor Author

seems like a nice fix! i think you may have to rebase to the current maintenance branch though

Sorry, I don't understand. The current maintenance branch is maintenance_1.4.x, no?

@filefolder
Copy link
Contributor

seems like a nice fix! i think you may have to rebase to the current maintenance branch though

Sorry, I don't understand. The current maintenance branch is maintenance_1.4.x, no?

ahh you're right sorry! i have lost track of the passage of time between versions.

@liamtoney
Copy link
Contributor Author

Hi folks, just checking if there is more feedback on this PR. Thanks.

Copy link
Contributor

@filefolder filefolder left a comment

Choose a reason for hiding this comment

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

looks good to me and tested fine on my machine

@liamtoney
Copy link
Contributor Author

looks good to me and tested fine on my machine

Great! Looks like this is ready to be merged, then? I can't do that part :)

@megies
Copy link
Member

megies commented Feb 29, 2024

sorry for the delay @liamtoney.. looks good to me, just two comments:

  1. would be nice to have a minimal regression test, easy to do with just a few lines using e.g. np.random.random_integers(-100, 100, 10010) as a single Trace's data and then plotting it and checking the resulting Line object, something like assert len(ax.lines[0].get_xdata()) == 10010 (to make sure point number is not reduced to max_npts.

  2. I was wondering why the min-max plot was looking that off from the original data.. turns out it's doing something different completely from the min-max routine on normal wiggle plot. I think it might be good to refactor the _plot_min_max routine a little bit to factor out the calculation routine for x/y values and then use the same routine in the section plot.

Would be nice to still get 1. in here, but 2. could be something for a different PR / another time.

@megies megies added this to the 1.4.1 milestone Feb 29, 2024
@megies megies added the .imaging Issues with our plotting functionalities label Feb 29, 2024
@liamtoney
Copy link
Contributor Author

2. I was wondering why the min-max plot was looking that off from the original data.. turns out it's doing something different completely from the min-max routine on normal wiggle plot. I think it might be good to refactor the _plot_min_max routine a little bit to factor out the calculation routine for x/y values and then use the same routine in the section plot.

Are you referring to the fact that the section plot resamples the data while the standard plot does the min/max? Maybe a separate PR for that?

@liamtoney
Copy link
Contributor Author

  1. would be nice to have a minimal regression test, easy to do with just a few lines using e.g. np.random.random_integers(-100, 100, 10010) as a single Trace's data and then plotting it and checking the resulting Line object, something like assert len(ax.lines[0].get_xdata()) == 10010 (to make sure point number is not reduced to max_npts.

Nice test idea! I implemented that and am ready for re-review, I think.

@megies
Copy link
Member

megies commented Mar 7, 2024

Are you referring to the fact that the section plot resamples the data while the standard plot does the min/max? Maybe a separate PR for that?

Yes, I was assuming it was doing the same min-max thing we have for the regular wiggle plot and was confused why the line was overshooting in some points and stuff. And yes, definitely something for another time, if at all. It's not a big deal really.

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.

Thanks for the fix! 🚀

@megies megies merged commit dd36ba0 into obspy:maintenance_1.4.x Mar 7, 2024
23 checks passed
@liamtoney liamtoney deleted the fix-section-method-full branch March 8, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.imaging Issues with our plotting functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants