DOI-USGS / DOI-USGS/streamMetabolizer
prepdata_bayes()'s "modal timestep" check doesn't do what its name/comment claim
- Dominant language
- Stan
- Stars
- 47
- Forks
- 25
- PR merge metrics
- No merged PRs in 30d
Description
`prepdata_bayes()` (in R/metab_bayes.R, around lines 492–498) has a block commented as confirming "every day has the same modal timestep," but the actual implementation does something different from what its name and comment claim — in three separate ways.
```
# confirm that every day has the same modal timestep and put a value on that
# timestep. ...
obs_times <- time_by_date_matrix(as.numeric(data$solar.time - data$solar.time[1], units='days'))
timestep_eachday <- apply(obs_times, MARGIN=2, FUN=mm_get_timestep, format='mean', require_unique=TRUE)
```
Problems:
1). "Modal" is wrong. `mm_get_timestep()` supports three format options: 'mean', 'unique', 'modal'. The comment says "modal," but the call passes `format='mean'`.
2). format='mean' doesn't actually compute a mean here. Looking at `mm_get_timestep()`'s implementation, `format='mean'` combined with `require_unique=TRUE` short-circuits to `format='unique'` behavior instead — i.e., it requires exactly one distinct timestep value (within `tol`) and returns that value. So despite what it says, this call is doing a strict uniqueness check, not averaging anything.
3). `tol` is never passed. The call above doesn't supply tol, so it silently falls back to `mm_get_timestep()`'s default (currently 60 seconds), rather than any value chosen deliberately for this context.
Why this hasn't caused visible problems: the package's existing test datasets (`data_metab()` synthetic data, French Creek reference data) all have sufficiently regular timesteps that the strict-uniqueness check passes without exposing the naming/behavior mismatch.
Proposed fix (not yet scoped in detail):
* Decide what this check should actually verify — most likely: each day's timestep is internally consistent (matches `format='unique'` behavior, which is arguably already what's happening, just mislabeled), or genuinely compute a per-day modal timestep if that's what is intended.
* Pass `tol` explicitly with a deliberate, documented value rather than relying on the undocumented default.
* Fix the comment to accurately describe whatever behavior is decided on.
* Add a direct unit test exercising this specific block with irregular-timestep synthetic data, since none of the current test datasets are irregular enough to exercise this path today — this is likely why the bug went unnoticed.
Originally flagged by @aappling-usgs in PR #469 review, from code inspection alone — confirmed empirically during a later refactor (PR #469, commit 5f3030e).
Contributor guide
Research direction
Start in R/metab_bayes.R around lines 492–498 and read mm_get_timestep() to establish the intended timestep rule and tolerance. Compare behavior with data_metab() and the French Creek reference data, then add an irregular-timestep synthetic unit test. Done means the chosen rule, explicit tol, and comment agree, and the test covers the check.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- r
- Domain
- data, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 45/100