Skip to content

Add plot-ts utility#102

Merged
lispandfound merged 22 commits intomasterfrom
plot_ts
Apr 27, 2025
Merged

Add plot-ts utility#102
lispandfound merged 22 commits intomasterfrom
plot_ts

Conversation

@lispandfound
Copy link
Contributor

This adds the plot-ts utility back to visualisation. I originally rewrote the ancient gmt-based plot-ts added it to workflow. Now, however, I realise it is more appropriately placed here because it is not a scientific computation. I had to rewrite it (again!) because the 3D-visualisation based on PyVista was too janky to work properly with all the cartographic features you actually want. Instead the actual video is generated using simple matplotlib animations with cartopy (rather than GMT) forming the geographic backend. Eventually I would prefer to use cartopy over GMT in the entire codebase.

Copilot AI review requested due to automatic review settings April 10, 2025 04:36
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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

@lispandfound
Copy link
Contributor Author

I have opted not to test the plot_ts module for two reasons,

  1. The utility is rarely used, and only for communication and sanity checking purposes, and
  2. It is slow and it would be extremely hard to check its output.

I have omitted it from code coverage

@lispandfound lispandfound requested a review from Copilot April 10, 2025 04:42
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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pyproject.toml:81

  • [nitpick] Plot_ts.py is currently excluded from test coverage. Consider adding tests to ensure its functionality or removing it from the omit list if coverage is desired.
omit = [
     "visualisation/plot_ts.py"
]

@lispandfound lispandfound requested a review from Copilot April 10, 2025 04:45
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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

pyproject.toml:81

  • The new plot_ts.py utility is omitted from test coverage. Consider adding tests for this file to ensure its functionality and avoid regressions.
omit = [
     "visualisation/plot_ts.py"
]

visualisation/plot_ts.py:371

  • Using set_array to update pcolormesh may not update the RGBA data correctly with shading='gouraud'. Consider using set_facecolor to update the quadmesh's facecolors for clarity.
pcm.set_array(apply_cmap_with_alpha(current_data, 0, max_motion, cmap=cmap))

@lispandfound lispandfound requested a review from Copilot April 10, 2025 22:55
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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

pyproject.toml:82

  • The new production code in visualisation/plot_ts.py is currently excluded from test coverage. Consider adding tests for this utility instead of omitting it.
     "visualisation/plot_ts.py"

joelridden
joelridden previously approved these changes Apr 11, 2025
@lispandfound
Copy link
Contributor Author

Despite saying I wouldn't, I've multi-processed this module. It provides a modest (2x) speedup on my computer. Have also added:

  1. Zoom controls because some simulations need to be zoomed in from their domain.
  2. OpenStreetMap tile support. If enabled, it uses tiles from OpenStreetMap instead of simple line drawing of the coast. Mostly useful for public communication purposes.

The multiprocessing somewhat complicated the logic. It now resembles something like the old plot-ts but it reliably works and provides time-to-completion estimates.

The x264 codec requires an even width and height. This fix adds black
pixels around the edges of the video to pad the width and height to an
even dimension. See https://stackoverflow.com/a/20848224.
@sungeunbae
Copy link
Member

As long as Numpydoc and Ruff are happy, I'm happy to approve

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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

pyproject.toml:82

  • The new plot_ts utility is excluded from coverage. Consider either adding tests for this module or removing it from the coverage omit list if it is production code.
     "visualisation/plot_ts.py"

@lispandfound lispandfound merged commit faacda5 into master Apr 27, 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.

5 participants