Bugfix for new plot "ObsFit (Active Observations Only)"#42
Bugfix for new plot "ObsFit (Active Observations Only)"#42ewhelan merged 5 commits intoHirlam:masterfrom
Conversation
Misbehaviour discovered by Reima and should now work as intended.
There was a problem hiding this comment.
Pull request overview
Fixes the data post-processing for the new timeseries plot “ObsFit (Active Observations Only)” so that RMS metrics are computed correctly from per-observation departures after filtering to active observations.
Changes:
- Add a local
.rms()helper to compute RMS with NA-safe handling (returnsNA_real_when a group has no non-NA values). - Simplify the post-processing by removing an intermediate, redundant grouping step and computing group summaries in a single
data.tableaggregation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dt <- dt[, .( | ||
| # total number of obs in this group | ||
| nobs_total = .N, | ||
| fg_rms_total = sqrt(sum((fg_dep - mean(fg_dep, na.rm=TRUE))^2, na.rm=TRUE) / sum(!is.na(fg_dep))), | ||
| an_rms_total = sqrt(sum((an_dep - mean(an_dep, na.rm=TRUE))^2, na.rm=TRUE) / sum(!is.na(an_dep))), | ||
| fg_rms_total = .rms(fg_dep), | ||
| an_rms_total = .rms(an_dep), | ||
| fg_bias_total = mean(fg_dep, na.rm=TRUE), | ||
| an_bias_total = mean(an_dep, na.rm=TRUE) |
There was a problem hiding this comment.
The updated RMS/bias aggregation for the new "ObsFit (Active Observations Only)" plot isn’t covered by tests. Consider adding a testthat case that feeds a small synthetic dataset through .obsFitActiveObsDataPostProcessingFunction and asserts: (1) only rows with an "active" status are included, (2) fg_rms_total/an_rms_total match the intended RMS formula, and (3) all-NA groups return NA_real_ instead of propagating unexpected values.
There was a problem hiding this comment.
Would it be neater to do this:
fg_rms_total = sqrt(mean(fg_dep^2, na.rm = TRUE))
an_rms_total = sqrt(mean(an_dep^2 na.rm = TRUE))Then, you do not need to define .rms.
| oldColAttrs <- lapply(names(data), function(nm) attributes(data[[nm]])) | ||
| names(oldColAttrs) <- names(data) | ||
|
|
||
There was a problem hiding this comment.
Remove this whitespace change please.
| dt <- dt[, .( | ||
| # total number of obs in this group | ||
| nobs_total = .N, | ||
| fg_rms_total = sqrt(sum((fg_dep - mean(fg_dep, na.rm=TRUE))^2, na.rm=TRUE) / sum(!is.na(fg_dep))), | ||
| an_rms_total = sqrt(sum((an_dep - mean(an_dep, na.rm=TRUE))^2, na.rm=TRUE) / sum(!is.na(an_dep))), | ||
| fg_rms_total = .rms(fg_dep), | ||
| an_rms_total = .rms(an_dep), | ||
| fg_bias_total = mean(fg_dep, na.rm=TRUE), | ||
| an_bias_total = mean(an_dep, na.rm=TRUE) |
There was a problem hiding this comment.
Would it be neater to do this:
fg_rms_total = sqrt(mean(fg_dep^2, na.rm = TRUE))
an_rms_total = sqrt(mean(an_dep^2 na.rm = TRUE))Then, you do not need to define .rms.
Removed unnecessary blank line before converting to data.table.
Misbehaviour discovered by Reima and should now work as intended.