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

Handle CubeList in line plotting operator #954

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Sylviabohnenstengel
Copy link
Contributor

@Sylviabohnenstengel Sylviabohnenstengel commented Dec 3, 2024

Fixes #918

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.

@Sylviabohnenstengel
Copy link
Contributor Author

addresses issue #918

@Sylviabohnenstengel Sylviabohnenstengel self-assigned this Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Coverage

@jfrost-mo jfrost-mo changed the title include cubeList in line plotting operator. Handle CubeList in line plotting operator Dec 3, 2024
@jfrost-mo jfrost-mo marked this pull request as ready for review December 3, 2024 14:50
@jfrost-mo jfrost-mo requested a review from afinnen December 3, 2024 14:50
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.

The right idea, the big change needed is that we can just return the original input data, as this function doesn't modify it.

Otherwise just some comments around variable naming and documentation.

Comment on lines +986 to +987
# To store plotted CubeList.
plotted_cubes = iris.cube.CubeList()
Copy link
Member

Choose a reason for hiding this comment

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

As this operator doesn't modify the data, we can just return the original thing we were given, rather than having to store it.

@@ -948,20 +948,21 @@ def spatial_pcolormesh_plot(
# Coordinate about which to plot multiple lines. Defaults to
# ``"realization"``.
def plot_line_series(
cube: iris.cube.Cube,
toplot: iris.cube.Cube | iris.cube.CubeList,
Copy link
Member

Choose a reason for hiding this comment

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

I don't like these runtogetherwords, as I read that as "top lot". Python code tends to use snake_case, where the words are separated with underscores.

The operators we already have that take either a Cube or a CubeList call this first arguments cubes, so perhaps we should continue that.

src/CSET/operators/plot.py Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Outdated Show resolved Hide resolved
Comment on lines +1020 to +1024
# Preserve returning a cube if only a cube has been supplied to regrid.
if len(plotted_cubes) == 1:
return plotted_cubes[0]
else:
return plotted_cubes
Copy link
Member

Choose a reason for hiding this comment

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

As above, we can skip this if we return the original cube(s).

@jfrost-mo
Copy link
Member

jfrost-mo commented Dec 3, 2024

Oops, I've realised I've reviewed this PR wrong. While this is the correct transformation for most of the operators that need converting to handle a CubeList, this particular one is a bit special.

When we give the line plot operator multiple cubes, I suspect we are wanting multiple lines.

Apologies for not being clear in the ticket.

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.

The main think that need to be done here now is to take the cubes into the internal plot_and_save function so each can be plotted as lines on the same axis.

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.

plot.plot_line_series should be able to handle a CubeList
2 participants