fix(plots): support DataArray and Dataset for y_obs in plot_lm (#454) - #543
fix(plots): support DataArray and Dataset for y_obs in plot_lm (#454)#543b25cs1051-KUSH wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
- Coverage 88.63% 88.61% -0.02%
==========================================
Files 64 64
Lines 7266 7273 +7
==========================================
+ Hits 6440 6445 +5
- Misses 826 828 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Documentation build overview
66 files changed ·
|
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
The PR description says:
For xr.DataArray / xr.Dataset: We now bypass extract() entirely, wrap the array cleanly, and align the names.
So far so good, the code shows extract is skipped and aligned observed_y as a Dataset and y_obs as a list of variables in that dataset always exit the if.
For str / list[str]: We use extract(), but then explicitly rename the target variable to align with the x_pred coordinates so the downstream plotting functions can digest it seamlessly.
I don't see any change to how str/list of str inputs behave being done in this PR.
In the hopes of seeing you contribute more often, we don't care about the prose quality/literacy of PR descriptions and comments. In fact, PR descriptions repeating the issue and describing the changes are generally useless. We need to (and will) review the changes even if it takes us a while. Therefore, the best PR descriptions focus on why those changes were done, known tradeoffs or limitations and things that might not be straightforward to parse from the code itself.
Here for example the key information seems to be:
- Naming issue was already fixed so no changes on that, there is also a test for that even
- DataArray input was still not handled so updated the code to support that and added a test
| @@ -328,16 +328,32 @@ def plot_lm( | |||
| y_obs = y | |||
| else: | |||
| y_obs = obs_vars[: len(y)] | |||
There was a problem hiding this comment.
we can move this inside the else to reduce the complexity of the if clauses. In pseudocode:
if DataArray:
...
if Dataset:
...
else:
if None:
...
if str:
...
extract()
|
Thanks for the feedback on PR descriptions. I will make sure that I follow the same as I continue to contribute... |
3 participants
Why these changes were made:
y_obswas resulting in a KeyError. Updated the code to support passing DataArrays directly and bypassingextract().(Note: The custom string naming issue was already resolved in a previous commit, so no changes were made to that logic).