Fix coords arg where only one coord can be specified for 2d plots #459
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue where only one coordinate can be specified for 2D plots by updating test cases and refining coordinate processing in the plotting backend.
- Updated tests to check proper error throwing when multiple coordinates for the same underlying dimension are provided.
- Modified the coordinate handling in common.py to drop unused coordinates and manage underlying dimensions.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/plotting/plot_2d_test.py | Added parameterized tests to validate single and duplicate coords usage. |
| src/plopp/plotting/common.py | Refactored coordinate validation and assignment logic to support the change. |
Comments suppressed due to low confidence (1)
src/plopp/plotting/common.py:99
- The removal of the explicit duplicate underlying coordinate check in the preprocess function (which formerly raised a ValueError) now relies on implicit behavior that leads to a KeyError in tests. Consider handling duplicate underlying dimensions explicitly to ensure consistent and clear error types throughout the API.
if underlying in underlying_dims:
jl-wynen
left a comment
There was a problem hiding this comment.
I find the logic difficult to follow. But the tests pass, so I guess it's correct.
src/plopp/plotting/common.py
Outdated
| for c in set(coords) - dims: | ||
| if c not in da.coords: | ||
| raise ValueError(f"Specified coords do not exist: {c}") | ||
| underlying = da.coords[c].dims[-1] |
There was a problem hiding this comment.
Why does this only look at the last dim? Is this a general convention in Plopp?
There was a problem hiding this comment.
I think in Plopp and Scipp?
If you have a 2d coord, the convention is that it is associated with the last dim.
Yes, on the train I thought that we should do it differently.
Would that cover everything? |
That was also why I asked copilot for a review, to see if it could follow the logic... |
Possibly. You'd need to still be careful around what to rename. |
Fixes #458