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.

Sign up for GitHub

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

Jump to bottom

Handle CubeLists in plot_vertical_line_series and plot_scatter_plot #1100

Merged
merged 9 commits into from
Feb 3, 2025
Merged

Handle CubeLists in plot_vertical_line_series and plot_scatter_plot #1100

merged 9 commits into from
Feb 3, 2025

Conversation

Sylviabohnenstengel
Copy link
Contributor

@Sylviabohnenstengel Sylviabohnenstengel commented Jan 29, 2025 •
edited
Loading

Contribution checklist

Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.

  • Documentation has been updated to reflect change.
  • New code has tests, and affected old tests have been updated.
  • All tests and CI checks pass.
  • Ensured the pull request title is descriptive.
  • Conda lock files have been updated if dependencies have changed.
  • Attributed any Generative AI, such as GitHub Copilot, used in this PR.
  • Marked the PR as ready to review.

Sorry, something went wrong.

@Sylviabohnenstengel Sylviabohnenstengel self-assigned this Jan 29, 2025
@Sylviabohnenstengel Sylviabohnenstengel added the enhancement New feature or request label Jan 29, 2025
@Sylviabohnenstengel
Copy link
Contributor Author

In order to get the plot operator working with lfric data I had to change the recipe for plotting on mlevels. this rquired changing the coordinate set to full_levels

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 29, 2025 •
edited
Loading

Coverage

@Sylviabohnenstengel
Copy link
Contributor Author

Due to rebasing main and then merging in those changes into my branch there are a lot of changes to review. the main change is in def _plot_and_save_vertical_line_series(

@Sylviabohnenstengel
Copy link
Contributor Author

Updating branch as this might help.

@Sylviabohnenstengel Sylviabohnenstengel marked this pull request as ready for review January 29, 2025 14:06
@Sylviabohnenstengel Sylviabohnenstengel mentioned this pull request Jan 29, 2025
Closed
@Sylviabohnenstengel Sylviabohnenstengel changed the title 919 plot vertical line series add cubelist handling 919 plot vertical line series and plot scatter plot add cubelist handling Jan 30, 2025
@Sylviabohnenstengel
Copy link
Contributor Author

used GitHub Copilot to include cubelists in zip function

@jfrost-mo jfrost-mo changed the title 919 plot vertical line series and plot scatter plot add cubelist handling Handle CubeLists in plot_vertical_line_series and plot_scatter_plot Feb 3, 2025
@jfrost-mo jfrost-mo changed the base branch from main to 920_handle_cubelist_plot_scatter_plot February 3, 2025 13:45
@jfrost-mo jfrost-mo changed the base branch from 920_handle_cubelist_plot_scatter_plot to main February 3, 2025 13:46
@jfrost-mo jfrost-mo force-pushed the 919_plot_vertical_line_series_add_cubelist_handling branch from b31db24 to efd89db Compare February 3, 2025 13:53
@jfrost-mo
Copy link
Member

I've done a fairly harsh rebase to remove the changes from other PRs. Its probably worth checking I haven't lost anything from it.

jfrost-mo
jfrost-mo reviewed Feb 3, 2025
View reviewed changes
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

Looks sensible. Some minor comments, but happy to re-review after they are fixed.

@Sylviabohnenstengel
Copy link
Contributor Author

@jfrost-mo addressed your comments and over to you for hopefully final check.

@Sylviabohnenstengel
Copy link
Contributor Author

Sylviabohnenstengel commented Feb 3, 2025 •
edited
Loading

the change to next(iter_maybe(cubes)).name() causes an error.

If it is not a cube then next does not work

This was linked to issues Feb 3, 2025
plot.plot_vertical_line_series should be able to handle a CubeList #919
Closed
plot.scatter_plot should be able to handle a CubeList #920
Closed
@jfrost-mo
Copy link
Member

Ah, that was my fault. It should have been iter_maybe(cubes)[0]

I'll fix it while I do my review.

@jfrost-mo
Correctly select the first cube
3402f6b
@jfrost-mo
Ensure scatter plot X and Y cubes are the same length
e47f9f8 
This prevents unexpectedly losing data when the operator is used improperly.
@jfrost-mo jfrost-mo force-pushed the 919_plot_vertical_line_series_add_cubelist_handling branch from 7708b8c to e47f9f8 Compare February 3, 2025 16:55
jfrost-mo
jfrost-mo approved these changes Feb 3, 2025
View reviewed changes
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

Looking good. I did do one other change, where I made the zip inside the scatter plot strict, as this ensures the x and y cubes have the same length.

@Sylviabohnenstengel Sylviabohnenstengel merged commit fa51bf4 into main Feb 3, 2025
8 checks passed
@Sylviabohnenstengel Sylviabohnenstengel deleted the 919_plot_vertical_line_series_add_cubelist_handling branch February 3, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfrost-mo jfrost-mo

Assignees

@Sylviabohnenstengel Sylviabohnenstengel

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
2 participants
@Sylviabohnenstengel @jfrost-mo