Skip to content

scaleModel() writes R_max/gamma directly into @species_params, bypassing given-value tracking #599

Description

@cmbeese

Problem

scaleModel() writes the rescaled R_max and gamma directly into the
@species_params slot instead of going through
species_params(params, recalculate = FALSE) <-. That bypasses mizer's
given-value tracking, so given_species_params() keeps the pre-scale
values while species_params() returns the post-scale ones. Any
subsequent call that triggers a recalculation of species parameters (even a
no-op species_params(p) <- species_params(p)) silently reverts R_max
(and gamma, where present) back to its pre-scale value, undoing the scale
factor without any warning.

This is the same failure mode matchGrowth() was fixed for (recording
scaled values via recalculate = FALSE so a later recalculation doesn't
undo the match) — scaleModel() doesn't have the equivalent fix.

Reprex

library(mizer)
packageVersion("mizer")
#> [1] '3.3.1'

p <- NS_params
before <- given_species_params(p)$R_max[1]

p2 <- scaleModel(p, factor = 2)
given_after <- given_species_params(p2)$R_max[1]
used_after  <- species_params(p2)$R_max[1]

cat("given R_max before scale: ", before, "\n")
cat("given R_max after scale:  ", given_after, " (unchanged -- not tracked)\n")
cat("used  R_max after scale:  ", used_after, " (correctly doubled)\n")
#> given R_max before scale:  7.38e+11
#> given R_max after scale:   7.38e+11
#> used  R_max after scale:   1.476e+12

# A later recalculation -- even a no-op -- silently reverts it:
p3 <- p2
species_params(p3) <- species_params(p3)
cat("used R_max after no-op recalculation: ", species_params(p3)$R_max[1], "\n")
#> used R_max after no-op recalculation:  7.38e+11

R_max silently drops back to its pre-scaleModel() value the moment
anything downstream triggers a species-parameter recalculation, with no
message or warning. search_vol scales the rate array directly (not
@species_params) so it isn't affected by this particular mechanism, but
gamma is written the same way as R_max and should have the same problem
whenever a model has a gamma column.

Where

scaleModel.MizerParams():

if ("R_max" %in% names(params@species_params)) {
    params@species_params$R_max <- params@species_params$R_max * factor
}
params@search_vol <- params@search_vol / factor
if ("gamma" %in% names(params@species_params)) {
    params@species_params$gamma <- params@species_params$gamma / factor
}

Suggested fix

Route these through the setter with recalculate = FALSE, the way
matchGrowth() already does, e.g.:

sp <- species_params(params)
if ("R_max" %in% names(sp)) {
    sp$R_max <- sp$R_max * factor
}
if ("gamma" %in% names(sp)) {
    sp$gamma <- sp$gamma / factor
}
species_params(params, recalculate = FALSE) <- sp

Context

Found while reviewing an mizerReef PR that upgrades to mizer 3.3 and fixes
this exact pattern (direct @species_params slot writes bypassing given-value
tracking) throughout mizerReef's own code, following the precedent set by
matchGrowth(). mizerReef's scaleReefModel() reproduces this block of
scaleModel() verbatim (by design, to stay in sync with mizer's own
behaviour), so it inherits the same bug -- confirmed with the same reprex
pattern against caribbean_3_model.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions