Skip to content

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

Description

@mdodrill-usgs

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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    Status
    No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions