-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: main
Are you sure you want to change the base?
Conversation
addresses issue #918 |
There was a problem hiding this 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.
# To store plotted CubeList. | ||
plotted_cubes = iris.cube.CubeList() |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
# 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 |
There was a problem hiding this comment.
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).
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. |
There was a problem hiding this 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.
Co-authored-by: James Frost <[email protected]>
Co-authored-by: James Frost <[email protected]>
Co-authored-by: James Frost <[email protected]>
Fixes #918
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.