Activity

1. aappling-usgs commented on Oct 13, 2026

   

   aappling-usgs

   on Oct 13, 2026

   Last edited by aappling-usgs

   CollaboratorAuthor

   More actions

   I'm working with Kiro to diagnose and make a plan, but I need to set this aside for a few days, so I asked Kiro to write up an issue description. Kiro and I iterated a bit to arrive here:

   convertToEGRET now breaks when accepting non-daily flow data after EGRET's populateDaily change

   Summary

   expandFlowForEGRET() errors when given non-daily flow data, failing six tests in test-51-EGRET.R. The error is a row-count mismatch in the final step:

   Error in `mutate(dateTime = flowdat[[date.colname]])`:
   ! `dateTime` must be size 4749 or 1, not 18996.
   

   This mutate() assumes, now incorrectly, that the output of populateDaily will have the same number of rows as the original flowdat.

   Root cause

   A recent change to EGRET::populateDaily() changed its row semantics. It now builds a complete daily date sequence (seq.Date(..., by = "day")) and left-joins the input onto it, returning one row per calendar day rather than one row per input row. (It does not aggregate; sub-daily values that don't align to a day boundary drop out or become NA.)

   expandFlowForEGRET() ends with:

   mutate(dateTime = flowdat[[date.colname]])

   which assumes populateDaily() returns one row per input row. That assumption was always fragile and is now broken for sub-daily input.

   Note

   This surfaced after updating the local EGRET install. In the prior test run (older EGRET build), expandFlowForEGRET() — including the mutate(dateTime = ...) line — completed successfully; the EGRET-related failures occurred later, in convertToEGRETDaily's prediction join. The row-count mismatch only appears with the updated populateDaily(). EGRET's new behavior is reasonable; loadflex just needs to adapt.

   The real problem

   expandFlowForEGRET() serves two callers with different granularity needs:

   • convertToEGRETDaily — output is the EGRET Daily frame, which by definition must be one row per day. The new populateDaily() behavior is correct here; only the trailing dateTime line is wrong.
   • convertToEGRETSample — only needs an instantaneous Q per sample observation (sub-daily OK), joined by dateTime. It immediately does select(Date, Q, dateTime), discarding all other populateDaily() columns. Routing it through the now-daily populateDaily() is both unnecessary and incorrect (breaks the sub-daily join).

   Proposed approach

   Split the two paths:

   1. Daily path: keep populateDaily() (daily output is correct for EGRET Daily). Drop the dateTime column entirely and key on Date instead — dateTime is not a native EGRET Daily column (EGRET's Daily keys on Date, class Date); it was only ever an internal join key. Update the prediction joins in convertToEGRETDaily to join on Date. Reject sub-daily input to the Daily path with an informative error (see note below).
   2. Sample path: stop calling expandFlowForEGRET(); compute Q = value / qconvert directly per observation (same qconvert = 1 / convertUnits(flow.units, "cms")), preserving one row per observation with instantaneous discharge. When joining the Sample frame to a Daily frame, convert the Sample dateTime to Date before merging.

   Confirmed EGRET's Sample frame tolerates irregular/sub-daily timestamps (populateSampleColumns() is row-preserving), so instantaneous per-observation Q is consistent with EGRET's data model.

   Timezone handling

   Dropping dateTime in favor of Date means POSIXct timestamps get reduced to calendar days via as.Date(), and as.Date(x) on a POSIXct defaults to tz = "UTC", which can shift a timestamp to the wrong calendar day relative to the series' own timezone.

   To keep this decision fully on loadflex's side rather than relying on populateDaily's internal date handling, convert the date column to class Date (with an explicit, consistent timezone) before passing it into populateDaily. Once the input key is already class Date, populateDaily's internal daily sequence (also class Date) merges against it cleanly — Q is preserved and genuine gaps are filled with NA. The same explicit-timezone as.Date() conversion is then used on the Sample side so the two frames' Date keys line up by construction.

   Note

   Pre-converting to Date assumes the Daily path's input is genuinely daily. If it contains multiple timestamps per day, the pre-conversion creates duplicate same-day keys and the merge emits one row per duplicate (not one per day, not aggregated). Since EGRET's Daily must be one row per day, the Daily path must reject sub-daily input with an informative error rather than silently duplicating rows. Detect it by checking for duplicate Date values after conversion (i.e., more than one row per calendar day).

   Tests

   Fixture: the shared test setup data — the objects (fitdat, estdat, meta, conc_lm, etc.) built at the top of test-51-EGRET.R before the test_that(...) blocks and reused across tests as known inputs.

   The current test-51-EGRET.R setup feeds sub-daily estdat (4 obs/day) into the Daily path. Under the new rules that input must error, so the fixtures and assertions need rework.

   Fixtures

   • Add a genuinely daily discharge fixture (e.g. a one-row-per-day estdat_daily) for the Daily-path tests.
   • Keep the sub-daily estdat around for the Sample path and for the new "rejects sub-daily" test.

   Daily path

   • convertToEGRETDaily tests: use the daily fixture; assert the result is one row per day (nrow == <n days>), not nrow(estdat).
   • Add a test that convertToEGRETDaily (and/or expandFlowForEGRET) errors informatively on sub-daily input.
   • expandFlowForEGRET test: switch to the daily fixture; update the nrow expectation to one-row-per-day.
   • Confirm the expected Daily columns no longer include dateTime (dropped), and update ncol/expected_cols assertions accordingly.

   Sample path

   • convertToEGRETSample tests: still row-per-observation (keyed on fitdat), so nrow == nrow(fitdat) should hold.
   • Verify the Sample-to-Daily prediction join now works on Date (not dateTime) and that timezone-consistent as.Date() conversion keeps sample rows matched to the correct day.

   Context to review before editing

   • Review the original intent behind the nrow(...) == nrow(estdat) assertions (git blame / surrounding comments) to confirm they encoded the old one-row-per-input assumption rather than a deliberate requirement.
2. aappling-usgs

   closed this as completedin #266on Oct 14, 2026
3. aappling-usgs

   added a commit that references this issue on Oct 14, 2026

   v1.9.4: populateDaily (#265), loadLm pred col (#258) (#266)

   0f6e7e6