Conversation
…gs cycle over the geos or the quantities in *_plt functions. The notebooks plotting_example.ipynb and pf_v_enkf.ipynb contain a number of examples. Note these notebooks should be removed before final merge.
|
I noticed some issues with the way I was deleting yticks from unused subplots. If |
|
The notebooks for testing various plots are linked below. |
| kwarg_type : | ||
| Whether to iterate the kwargs over the quantities or the geos. | ||
| Options are "geo", default is quantity iteration. |
There was a problem hiding this comment.
When a parameter accepts a small set of string literals, we should type it narrowly than str. In this case: Literal["quantity", "geo"]. You can make that a named type if that helps readability.
For the sake of consistent docs, I'd prefer to use a bullet list containing all of the options.
The legend parameter from plot.line() is an example of both of the above.
| ax_title : | ||
| A format string to display as the title for each subplot. | ||
| Defaults to displaying the geo. |
There was a problem hiding this comment.
I know this feels redundant, but this doc string (and any others that use format strings) should be explicit about which replacement variables are supported. Again, refer to plot.line().
| "auto" will use the format defined by the grouping of the `time` parameter, | ||
| "date" attempts to display calendar dates, | ||
| "day" attempts to display days numerically indexed from the start of the | ||
| simulation with the first day being 0. |
There was a problem hiding this comment.
This also needs bullet-list markup.
| if leg is not None: | ||
| for lh in leg.legend_handles: | ||
| lh.set_alpha(1) |
There was a problem hiding this comment.
Is there a reason to wait to modify the legend down here, or can we move this block up closer to where leg was declared?
Also, Pylance is telling me the legend handles list can contain None. This is probably just matplotlib being sloppy with types, but should probably do one more if-check for safety.
| geo_indices = np.array(geo.indices) | ||
|
|
||
| if geo.grouping is None: | ||
| if geo.aggregation is None: | ||
| num_nodes = self.output.rume.scope.nodes | ||
| else: | ||
| num_nodes = 1 | ||
|
|
||
| else: | ||
| num_nodes = len( | ||
| np.unique(geo.grouping.map(geo.scope.node_ids[geo_indices])) | ||
| ) |
There was a problem hiding this comment.
This logic occurs four times. Good candidate to move to a helper function?
JavadocMD
left a comment
JavadocMD
left a comment
There was a problem hiding this comment.
Whoops, I forgot to "start review" with the above comments, but see above comments. I think some of the guidance on docstring style should be applied broadly across the different plot functions.
3 participants
This branch contains the KDE plot and tweaks allowing the user to choose whether to cycle the kwargs over the geo or quantity. The notebooks plotting_example.ipynb and pf_v_enkf.ipynb show a number of examples of plots in action.
Note these notebooks need to be removed before merge commit.