DOI-USGS / DOI-USGS/streamMetabolizer

prepdata_bayes()'s "modal timestep" check doesn't do what its name/comment claim

Open
#474 0 comments 0 reactions 0 assignees View on GitHub
CDI26
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.