Skip to content

Allow plot-ts to plot SRFs as animations#103

Merged
lispandfound merged 14 commits intomasterfrom
plot_ts_srf
May 6, 2025
Merged

Allow plot-ts to plot SRFs as animations#103
lispandfound merged 14 commits intomasterfrom
plot_ts_srf

Conversation

@lispandfound
Copy link
Contributor

This PR extends plot-ts to allow it to visualise an SRF as an animation (in addition to it's ability to plot xyts files). For example,

$ plot-ts srf alpine_wairau_1.json alpine_wairau_1_modified.srf modified.mp4 --simple-map --fps 5

The old behaviour is obtained with the xyts subcommand

$ plot-ts xyts alpine_wairau_1.json alpine_wairau_1_xyts.e3d modified_xyts.mp4 --simple-map --fps 5

In accordance with the last PR, this SRF plot-ts is not explicitly tested (because I can't think of a good way to do this).

Copilot AI review requested due to automatic review settings April 29, 2025 03:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends plot-ts to support rendering SRF animations alongside the existing xyts visualisations. Key changes include:

  • Introducing a new command for SRF animations (animate_srf_slip_times) along with supporting helper functions.
  • Modifying the waveform coordinate computation and reorganising imports for clarity.
  • Adjusting the frame rendering logic in both xyts and SRF animation pathways.
Comments suppressed due to low confidence (2)

visualisation/plot_ts.py:293

  • The parameter 'ny' is described as 'the number of x-points'; it should likely be corrected to 'the number of y-points' for clarity.
ny : int

visualisation/plot_ts.py:822

  • [nitpick] The magic number '20' used for calculating slip_index is hardcoded. Consider defining it as a named constant to explain its significance and facilitate future adjustments.
slip_index = frame_index * 20

@lispandfound lispandfound requested a review from Copilot April 29, 2025 04:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the plot-ts utility to support SRF animation rendering alongside the existing XYTS plotting, and refactors waveform coordinate computation. Key changes include:

  • Refactoring of xyts_waveform_coordinates to waveform_coordinates with explicit grid parameters.
  • Addition of a new Typer CLI command "srf" to animate SRF slip times.
  • Rearrangement of certain imports and modification of the frame rendering process in the low frequency animation function.
Comments suppressed due to low confidence (1)

visualisation/plot_ts.py:459

  • The render_single_frame function in the low frequency animation now returns a list containing the Axes object instead of saving the generated frame to a file, which may break the subsequent ffmpeg processing expecting image file paths. Consider restoring the functionality to save the frame as an image and returning the corresponding filename.
return [ax]

@lispandfound
Copy link
Contributor Author

Needed to bump scipy's version to at least 1.15.0 because it needs the sparse indexing operations only supported in the newest stable version.

@lispandfound lispandfound requested a review from Copilot April 29, 2025 22:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends plot-ts with the ability to animate SRFs while preserving the original xyts plotting functionality. Key changes include refactoring the waveform coordinate computation function, introducing a new CLI command "srf" with its own animation routine, and adding helper functions for processing non-zero data points.

@lispandfound lispandfound requested a review from Copilot April 29, 2025 23:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends plot-ts to support visualizing SRF files as animations alongside the existing xyts file plotting functionality. Key changes include:

  • Addition of a new command (animate_srf_slip_times) for rendering SRF animations.
  • Refactoring of waveform coordinate computation to decouple from XYTS-specific logic.
  • Adjusted import order and consolidation of redundant import statements.
Comments suppressed due to low confidence (1)

visualisation/plot_ts.py:731

  • [nitpick] The attribute name 'slipt1_array' is ambiguous and may be a typo; please verify its intended naming for clarity.
slip = srf_file.slipt1_array.tocsc()

@lispandfound lispandfound requested a review from Copilot April 30, 2025 01:46
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the functionality of plot‑ts by adding support for animating SRF slip times, while still retaining the existing XYTS animation capability.

  • Renames and refactors coordinate computation for waveform plotting.
  • Introduces a new subcommand for SRF animations with dedicated rendering logic.
  • Updates and reorders several imports and adjusts parameter types (dpi, fps) for consistency.

@lispandfound
Copy link
Contributor Author

The tests are failing due to a bug in openquake. It currently has a dependency requirement of numpy > 1.20, which results in numpy > 2 being installed and openquake doesn't work with numpy > 2. This is all due to a backend requirement in workflow, which visualisation depends on for some plotting utilities related to realisations. The best fix is openquake ports to numpy 2 (ongoing). The second best fix is pinning empirical engine to a numpy version less than 2, which is what I am going to do now.

Co-authored-by: AndrewRidden-Harper <52001209+AndrewRidden-Harper@users.noreply.github.com>
Copy link
Contributor

@joelridden joelridden 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, a rerun of the Jenkins test should work now that Empirical engine has updated and as long as that's passes I'm happy.

@lispandfound lispandfound merged commit b7517c1 into master May 6, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants