Skip to content

Address issues 259 260 261 - #264

Merged
aappling-usgs merged 3 commits into
mainfrom
259-260-261
Oct 14, 2026
Merged

aappling-usgs merged 3 commits into
mainfrom
259-260-261

Conversation

@aappling-usgs

@aappling-usgs aappling-usgs commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

This PR resolves a few easy-to-fix warnings and errors, as described in the following issues:

  • resolves getCorrectionFraction giving Inf #259: getCorrectionFraction was returning Inf rather than a number - that's fixed now.
  • resolves predictSolute(no3_li) giving warning with regularize.values #260: predictSolute.loadInterp() was giving a warning related to stats::approx which calls stats:::regularize.values() - investigation uncovered an incorrect selection of left bounds for the horizontal lines in a rectangular interpolation, which is now fixed.
  • resolves the easy parts of inconsistent count in predictSolute.loadReg2 output #261: I thought predictSolute was being inconsistent in returning count, count.days, or no count column across various models. It turns out it was intentional for predictSolute.loadReg2 to return count.days as already described in the doc for that method, and I fixed an inconsistency in predictSolute.loadInterp's default for the count argument relative to other models (though behavior is unchanged), and I fixed an error when requesting predictSolute.loadReg2(agg.by="day") that happened to occur when trying to create the count.days column. I left the potentially harder work in inconsistent count in predictSolute.loadReg2 output #261 for another day, as documented in that issue's final comment.

These warnings, errors, and inconsistencies became visible as I was updating the vignette; I'll include a separate PR for the vignette changes. The GitHub action to build the pkgdown site should complete successfully, but it'll display an old version of the vignette.

@github-actions

Copy link
Copy Markdown

📖 pkgdown preview for this PR: https://doi-usgs.github.io/loadflex/preview/pr264/

@aappling-usgs

aappling-usgs commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator Author

Running tests with my temp scripts.

r$> source('temp/run-tests.R')
r$> run_tests(phase_completed='Edits for PR #264, "Address issues 259 260 261"')

r$> source('temp/compare-tests.R')
r$> compare_edits(old_file='temp/test-output-261005-1736.rds', new_file='temp/test-output-261013-1102.rds')
OLD: temp/test-output-261005-1736.rds (Phase 2 complete — tidyverse-drift fixes applied)
    Tests: 89 | Passed: 41 | Failed: 61 | Errors: 16 | Warnings: 21 | Skipped: 24
NEW: temp/test-output-261013-1102.rds (Edits for PR #264, "Address issues 259 260 261")
    Tests: 89 | Passed: 35 | Failed: 61 | Errors: 22 | Warnings: 21 | Skipped: 24
MODE: structured (.rds sidecars found)

========================================================
  FAILURES (structured, matched by file + test)
========================================================
Totals: OLD = 61 across 10 tests | NEW = 61 across 10 tests

IMPROVED - FAILURES fully gone from these tests
-----------------------------------------------
(none)

DEGRADED - FAILURES newly present in these tests
------------------------------------------------
(none)

IMPROVED - fewer FAILURES in these tests
----------------------------------------
(none)

DEGRADED - more FAILURES in these tests
---------------------------------------
(none)

========================================================
  ERRORS (structured, matched by file + test)
========================================================
Totals: OLD = 16 across 16 tests | NEW = 22 across 22 tests

IMPROVED - ERRORS fully gone from these tests
---------------------------------------------
(none)

DEGRADED - ERRORS newly present in these tests
----------------------------------------------
test-44-plots.R - plotEGRET works for fitdat and meta (now 1)

test-51-EGRET.R - convertToEGRET allows metadata and data or newdata without model (now 1)

test-51-EGRET.R - convertToEGRET complains about redundant metadata or data (now 1)

test-51-EGRET.R - convertToEGRETDaily returns partial info without preds (now 1)

test-51-EGRET.R - convertToEGRETSample correctly converts (now 1)

test-51-EGRET.R - expandFlowForEGRET returns correct columns (now 1)


IMPROVED - fewer ERRORS in these tests
--------------------------------------
(none)

DEGRADED - more ERRORS in these tests
-------------------------------------
(none)

========================================================
  WARNINGS (structured, matched by file + test)
========================================================
Totals: OLD = 21 across 15 tests | NEW = 21 across 15 tests

IMPROVED - WARNINGS fully gone from these tests
-----------------------------------------------
(none)

DEGRADED - WARNINGS newly present in these tests
------------------------------------------------
(none)

IMPROVED - fewer WARNINGS in these tests
----------------------------------------
(none)

DEGRADED - more WARNINGS in these tests
---------------------------------------
(none)

ALL SIX of these new errors are stopping at the same line:

  ├─loadflex:::expandFlowForEGRET(...)
  │ └─... %>% mutate(dateTime = flowdat[[date.colname]]) at loadflex/R/convertToEGRET.R:240:3

with the same general error message, though with differing expected and actual dateTime sizes:

! `dateTime` must be size 4740 or 1, not 555.         # 4 of these
! `dateTime` must be size 4749 or 1, not 18996.     # 2 of these

I currently believe that something important in EGRET changed, because the hash of EGRET that I was using had changed from the previous PR to this one:

    EGRET           3.0.12     2026-02-13 [1] Github (DOI-USGS/EGRET@99d9ffd) # last time
    EGRET           3.0.12     2026-10-12 [1] Github (DOI-USGS/EGRET@b5eb22c) # today

And when I install the older @99d9ffd version and rerun tests, I can get rid of the errors without reverting this PR.

THEREFORE I plan to address these new EGRET-driven errors in another issue and future PR.

Details Here are more details on the new errors:

test-44-plots.R - plotEGRET works for fitdat and meta (now 1)

── 75. Error ('test-44-plots.R:26:3'): plotEGRET works for fitdat and meta ──────────────────────────────────────────────────────────────────
<dplyr:::mutate_error/rlang_error/error/condition>
Error in `mutate(., dateTime = flowdat[[date.colname]])`: i In argument: `dateTime = flowdat[[date.colname]]`.
Caused by error:
! `dateTime` must be size 4740 or 1, not 555.
Backtrace:
     ▆
  1. ├─loadflex::plotEGRET(plot.name = "plotConcQ", data = fitdat, meta = meta) at test-44-plots.R:26:3
  2. │ └─loadflex::convertToEGRET(load.model, newdata, data, meta) at loadflex/R/plotEGRET.R:122:5
  3. │   └─loadflex:::convertToEGRETSample(data, meta, daily_df) at loadflex/R/convertToEGRET.R:43:3
  4. │     └─... %>% select(Date, Q, dateTime) at loadflex/R/convertToEGRET.R:101:3
  5. ├─dplyr::select(., Date, Q, dateTime)
  7. ├─loadflex:::expandFlowForEGRET(...)
  8. │ └─... %>% mutate(dateTime = flowdat[[date.colname]]) at loadflex/R/convertToEGRET.R:240:3
  9. ├─dplyr::mutate(., dateTime = flowdat[[date.colname]])
  10. ├─dplyr:::mutate.data.frame(., dateTime = flowdat[[date.colname]])

test-51-EGRET.R - convertToEGRET allows metadata and data or newdata without model (now 1)

── 78. Error ('test-51-EGRET.R:35:3'): convertToEGRET allows metadata and data or newdata without model ─────────────────────────────────────
<dplyr:::mutate_error/rlang_error/error/condition>
Error in `mutate(., dateTime = flowdat[[date.colname]])`: i In argument: `dateTime = flowdat[[date.colname]]`.
Caused by error:
! `dateTime` must be size 4740 or 1, not 555.
Backtrace:
     ▆
  1. ├─loadflex:::convertToEGRET(data = fitdat, meta = meta) at test-51-EGRET.R:35:3
  2. │ └─loadflex:::convertToEGRETSample(data, meta, daily_df) at loadflex/R/convertToEGRET.R:43:3
  3. │   └─... %>% select(Date, Q, dateTime) at loadflex/R/convertToEGRET.R:101:3
  4. ├─dplyr::select(., Date, Q, dateTime)
  5. ├─loadflex:::expandFlowForEGRET(...)
  6. │ └─... %>% mutate(dateTime = flowdat[[date.colname]]) at loadflex/R/convertToEGRET.R:240:3
  7. ├─dplyr::mutate(., dateTime = flowdat[[date.colname]])
  8. ├─dplyr:::mutate.data.frame(., dateTime = flowdat[[date.colname]])

test-51-EGRET.R - convertToEGRET complains about redundant metadata or data (now 1)

── 77. Error ('test-51-EGRET.R:28:3'): convertToEGRET complains about redundant metadata or data ────────────────────────────────────────────
<dplyr:::mutate_error/rlang_error/error/condition>
Error in `mutate(., dateTime = flowdat[[date.colname]])`: i In argument: `dateTime = flowdat[[date.colname]]`.
Caused by error:
! `dateTime` must be size 4740 or 1, not 555.
Backtrace:
     ▆
  1. ├─testthat::expect_warning(...) at test-51-EGRET.R:28:3
  2. │ └─testthat:::expect_condition_matching_(...)
  3. │   └─testthat:::quasi_capture(...)
  4. │     ├─testthat (local) .capture(...)
  5. │     │ └─base::withCallingHandlers(...)
  6. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
  7. ├─loadflex:::convertToEGRET(data = fitdat, load.model = conc_lm)
  8. │ └─loadflex:::convertToEGRETSample(data, meta, daily_df) at loadflex/R/convertToEGRET.R:43:3
  9. │   └─... %>% select(Date, Q, dateTime) at loadflex/R/convertToEGRET.R:101:3
 11. ├─dplyr::select(., Date, Q, dateTime)
 12. ├─loadflex:::expandFlowForEGRET(...)
 13. │ └─... %>% mutate(dateTime = flowdat[[date.colname]]) at loadflex/R/convertToEGRET.R:240:3
 14. ├─dplyr::mutate(., dateTime = flowdat[[date.colname]])
 15. ├─dplyr:::mutate.data.frame(., dateTime = flowdat[[date.colname]])

test-51-EGRET.R - convertToEGRETDaily returns partial info without preds (now 1)

── 81. Error ('test-51-EGRET.R:89:3'): convertToEGRETDaily returns partial info without preds ───────────────────────────────────────────────
<dplyr:::mutate_error/rlang_error/error/condition>
Error in `mutate(., dateTime = flowdat[[date.colname]])`: i In argument: `dateTime = flowdat[[date.colname]]`.
Caused by error:
! `dateTime` must be size 4749 or 1, not 18996.
Backtrace:
     ▆
  1. ├─loadflex:::convertToEGRETDaily(meta = meta, newdata = estdat) at test-51-EGRET.R:89:3
  2. │ └─loadflex:::expandFlowForEGRET(...) at loadflex/R/convertToEGRET.R:176:3
  3. │   └─... %>% mutate(dateTime = flowdat[[date.colname]]) at loadflex/R/convertToEGRET.R:240:3
  4. ├─dplyr::mutate(., dateTime = flowdat[[date.colname]])
  5. ├─dplyr:::mutate.data.frame(., dateTime = flowdat[[date.colname]])

test-51-EGRET.R - convertToEGRETSample correctly converts (now 1)

── 80. Error ('test-51-EGRET.R:73:3'): convertToEGRETSample correctly converts ──────────────────────────────────────────────────────────────
<dplyr:::mutate_error/rlang_error/error/condition>
Error in `mutate(., dateTime = flowdat[[date.colname]])`: i In argument: `dateTime = flowdat[[date.colname]]`.
Caused by error:
! `dateTime` must be size 4740 or 1, not 555.
Backtrace:
     ▆
  1. ├─loadflex:::convertToEGRETSample(meta = meta, data = fitdat) at test-51-EGRET.R:73:3
  2. │ └─... %>% select(Date, Q, dateTime) at loadflex/R/convertToEGRET.R:101:3
  3. ├─dplyr::select(., Date, Q, dateTime)
  4. ├─loadflex:::expandFlowForEGRET(...)
  5. │ └─... %>% mutate(dateTime = flowdat[[date.colname]]) at loadflex/R/convertToEGRET.R:240:3
  6. ├─dplyr::mutate(., dateTime = flowdat[[date.colname]])
  7. ├─dplyr:::mutate.data.frame(., dateTime = flowdat[[date.colname]])

test-51-EGRET.R - expandFlowForEGRET returns correct columns (now 1)

── 83. Error ('test-51-EGRET.R:106:3'): expandFlowForEGRET returns correct columns ──────────────────────────────────────────────────────────
<dplyr:::mutate_error/rlang_error/error/condition>
Error in `mutate(., dateTime = flowdat[[date.colname]])`: i In argument: `dateTime = flowdat[[date.colname]]`.
Caused by error:
! `dateTime` must be size 4749 or 1, not 18996.
Backtrace:
     ▆
  1. ├─loadflex:::expandFlowForEGRET(estdat, "DISCHARGE", "DATE", "ft^3 s^-1") at test-51-EGRET.R:106:3
  2. │ └─... %>% mutate(dateTime = flowdat[[date.colname]]) at loadflex/R/convertToEGRET.R:240:3
  3. ├─dplyr::mutate(., dateTime = flowdat[[date.colname]])
  4. ├─dplyr:::mutate.data.frame(., dateTime = flowdat[[date.colname]])

@gfkltn gfkltn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admin review completed. No issues found.

@aappling-usgs
aappling-usgs merged commit 52cbe9d into main Oct 14, 2026
1 check passed
github-actions Bot pushed a commit that referenced this pull request Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

predictSolute(no3_li) giving warning with regularize.values getCorrectionFraction giving Inf

2 participants