Skip to content

Commit 86275ea

Browse files
committed
refactor(align.met tests): apply reviewer feedback from infotroph
- Remove skip_if_not_installed() guards: ncdf4, withr, and lubridate are required dependencies for this test; missing them should cause a hard failure, not a silent skip. - Simplify the verbose bug-context comment to a one-liner focused on the deliberate choice of stamps.hr[1] == 0.5. - Replace seed = 42 with seed = 20260602 in both tests. - Remove custom label = "..." arguments from expect_equal() calls; labels are documented as "for expert use only" and were redundant here. - Shorten the single-series test by removing the implementation detail comment about which code path is exercised. - Update NEWS.md entry to the concise format suggested by infotroph.
1 parent dc480b5 commit 86275ea

2 files changed

Lines changed: 9 additions & 28 deletions

File tree

modules/data.atmosphere/NEWS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
## Fixed
44

5-
* `align.met()`: fixed a silent data-corruption bug in the ensemble-source code path. When the source data was at a coarser time step than the training data (`align == "repeat"`), `rep(dat.tem, each = stamps.hr)` passed the full `stamps.hr` vector as the `each` argument. R silently uses only the first element in that case, producing the wrong replication count (half the training hour-step rather than the number of output sub-steps per source step). The fix matches the equivalent single-time-series path: `each = length(stamps.hr)`.
5+
* `align.met()` now correctly calculates output timesteps in cases where the source data is at a coarser time step than the training data and `align == "repeat"` (#4012, @anshul23102).
66

77
# PEcAn.data.atmosphere 1.9.1
88

modules/data.atmosphere/tests/testthat/test.align_met.R

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -35,17 +35,11 @@ make_align_nc <- function(n_time, outdir, filename) {
3535
}
3636

3737
test_that("align.met ensemble source path produces non-empty source data (bug: each=0 from stamps.hr truncation)", {
38-
skip_if_not_installed("ncdf4")
39-
skip_if_not_installed("withr")
40-
skip_if_not_installed("lubridate")
41-
4238
train_dir <- withr::local_tempdir()
4339
source_dir <- withr::local_tempdir()
4440

45-
## Training: hourly, 8760 time steps for 2001 (365 * 24).
46-
## This produces stamps.hr[1] = 0.5 in the ensemble source path.
47-
## Before the fix, 0.5 truncates to 0 and rep(..., each=0) returns an
48-
## empty vector, silently zeroing out all source data.
41+
## Hourly training (8760 steps) produces stamps.hr[1] == 0.5, intentionally
42+
## chosen so that any truncation of the step count to integer yields 0.
4943
make_align_nc(n_time = 8760, outdir = train_dir, filename = "2001.nc")
5044

5145
## Source: daily (365 steps), placed inside an ensemble subfolder.
@@ -57,42 +51,29 @@ test_that("align.met ensemble source path produces non-empty source data (bug: e
5751
train.path = train_dir,
5852
source.path = source_dir,
5953
n.ens = 1,
60-
seed = 42
54+
seed = 20260602
6155
)
6256

63-
## Training should have 8760 rows (1 per hour).
64-
expect_equal(nrow(result$dat.train$air_temperature), 8760,
65-
label = "training row count is 8760 (hourly)")
66-
67-
## Before the fix, rep(..., each=0) silently produced an empty matrix, so
68-
## dat.source would have 0 rows. After the fix, each daily source value is
69-
## carried through (repeated once), giving 365 rows.
70-
expect_equal(nrow(result$dat.source$air_temperature), 365,
71-
label = "source data is non-empty after fix (was 0 rows before)")
57+
## 8760 hourly training rows; 365 source rows (one per day, repeated once each).
58+
expect_equal(nrow(result$dat.train$air_temperature), 8760)
59+
expect_equal(nrow(result$dat.source$air_temperature), 365)
7260
})
7361

7462
test_that("align.met single-series source matches training row count when already aligned", {
75-
skip_if_not_installed("ncdf4")
76-
skip_if_not_installed("withr")
77-
skip_if_not_installed("lubridate")
78-
7963
train_dir <- withr::local_tempdir()
8064
source_dir <- withr::local_tempdir()
8165

8266
## Both training and source at the same 3-hourly resolution (2920 steps for 2001).
83-
## Source files are placed directly in source_dir (single-series path), not in
84-
## a subdirectory, so the already-correct line 304 is used.
8567
make_align_nc(n_time = 2920, outdir = train_dir, filename = "2001.nc")
8668
make_align_nc(n_time = 2920, outdir = source_dir, filename = "2001.nc")
8769

8870
result <- align.met(
8971
train.path = train_dir,
9072
source.path = source_dir,
9173
n.ens = 1,
92-
seed = 42
74+
seed = 20260602
9375
)
9476

9577
expect_equal(nrow(result$dat.source$air_temperature),
96-
nrow(result$dat.train$air_temperature),
97-
label = "source and training row counts match when already aligned")
78+
nrow(result$dat.train$air_temperature))
9879
})

0 commit comments

Comments
 (0)