Refactor audit

[kylebaron]

Summary

When a model contains ODE, we need an equation written for every compartment in $ODE block. "audit" means we look for some evidence that every compartment has been referenced somewhere.

The audit code was totally different when nm-vars plugin was invoked compared to when it wasn't invoked.

• nm-vars: ODE use DADT(n) = on the lhs
• non-nm-vars: ODE use dxdt_CMT = on the lhs

The code for checking in the two different approaches was different because there is special handling for nm-vars style and it was a little easier at the time to just keep that down in the code where all the nm-vars functionality was handled.

One bad decision from nm-vars is that we would only audit if there was at least one DADT(n) found. That wasn't safe because #1182 is a report from a user who invoked nm-vars, but used the non-nm-vars style, which is allowed. And of course the name of a dxdt_CMT variable was misspelled. The missing ODE wasn't detected and we spent some time trying to figure that out.

So here we centralize checking $ODE for the required lhs.

[kyleam]

Hmm, so there are some cases in user code where what would have been an error will now just be a warning after this change?

[kylebaron]

Yes. I must have been on edge when writing the NM stuff.

What about reversing this and making it an error all the time now? I was wondering about this ... it probably should be an error and users can opt out from this audit in fringe cases where each compartment isn't explicitly written into the $ODE block (like writing in a loop).

What do you think?

[kyleam]

Thanks for confirming. I don't have a strong leaning there, and don't have any objections to going with the warning if you prefer to go with the less aggressive behavior when consolidating the nm-vars and non-nmvars checks.

[kylebaron]

@kyleam I didn't see this warning before? https://github.com/metrumresearchgroup/mrgsolve/actions/runs/11194343722/job/31120704962?pr=1235#step:6:57

But can't say I looked either.

[kyleam]

Thanks for the updates. Looking good to me.

Left an updated suggestion for the grepl_dxdt_ode regex for you consideration.

[kyleam]

Thanks for confirming. I don't have a strong leaning there, and don't have any objections to going with the warning if you prefer to go with the less aggressive behavior when consolidating the nm-vars and non-nmvars checks.

[kyleam]

@kyleam I didn't see this warning before? https://github.com/metrumresearchgroup/mrgsolve/actions/runs/11194343722/job/31120704962?pr=1235#step:6:57

Warning: Dependence on R version ‘3.6.2’ not with patchlevel 0

Yeah, I have noticed that showing up under the annotations of the CI summary. An example from 4 months ago:
https://github.com/metrumresearchgroup/mrgsolve/actions/runs/9762229644

Here are the relevant docs and comments for this check:

@item _R_CHECK_R_DEPENDS_
Check that any dependence on R is not on a recent patch-level version
such as @code{R (>= 3.3.3)} since blocking installation of a package
will also block its reverse dependencies.  Possible values
@samp{"note"}, @samp{"warn"} and logical values (where currently true
values are equivalent to @samp{"note"}).
Default: false (but @samp{"warn"} for @option{--as-cran}).

## Dependence on say R >= 3.4.3 when 3.4 is current can
## cause problems with revdeps (and did for 3.2.x).
## We only check recent ones: maybe previous two
## (R-release and R-old-release) while this is R-devel

https://github.com/wch/r-source/blob/e48723ec1a1a49f62de4b637b8f82439ec6e52d2/src/library/tools/R/check.R#L1205-L1235

Given these are jobs running older R versions, I think we can safely ignore the warning. We could set _R_CHECK_R_DEPENDS_=FALSE (perhaps just for jobs other than r: release) to avoid the warning.

[kylebaron]

warning versus error: if I was doing this new today, I'd probably make it an error. But I think the majority of users have been getting a warning so will keep it at that for now. But I'm going to open an issue about transitioning that at some point ... it's probably worth considering again.