Replace ProSpectSEDlike manual parameter plumbing with ParmOff#31
Conversation
Agent-Logs-Url: https://github.com/asgr/ProSpect/sessions/20970253-fcc0-468d-9a54-4254b92e17f4 Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
asgr
left a comment
There was a problem hiding this comment.
All good with the switch to ParmOff
There was a problem hiding this comment.
Pull request overview
This PR refactors ProSpectSEDlike() to delegate parameter bounds and log-space transforms, plus ProSpectSED argument dispatch, to ParmOff, aiming to remove manual parameter “plumbing” and better support logical-vector .logged.
Changes:
- Added a
ParmOff-based preprocessing step to generate the final scalar parameter vector passed intoProSpectSED. - Replaced both
do.call("ProSpectSED", ...)sites withParmOff::ParmOff(..., .return = "func")to centralize argument matching/filtering. - Updated photo-z “z is logged” handling to use name-based
logged_parm_names.
Comments suppressed due to low confidence (2)
R/ProSpect.R:644
scat_scaleextraction currently usesparmlistbeforeparmlistis created by theParmOff::ParmOff(..., .return='args')call, which will error on the first evaluation whenscat_scaleis inData$parm.names. Extractscat_scalefrom the (possibly constrained/bounded/logged) parameter vector after it’s been processed, or processscat_scaleseparately before building the ProSpectSED argument list.
This issue also appears on line 649 of the same file.
if('scat_scale' %in% Data$parm.names){
sel = which('scat_scale' == Data$parm.names)
scat_scale = parmlist[sel]
parmlist = parmlist[-sel]
Data$parm.names = Data$parm.names[-sel]
R/ProSpect.R:656
- After removing
scat_scalefromData$parm.names,parm_lower/parm_upperare still created fromData$intervals$lo/hi(which likely still includescat_scale). Assigningnames(parm_lower) = Data$parm.nameswill fail if the lengths differ. Buildparm_lower/upperfrom the same set of parameters you intend to pass toParmOff(or dropscat_scalefrom the intervals before converting/naming).
parm_lower = NULL
parm_upper = NULL
if (!is.null(Data$intervals)) {
parm_lower = as.list(Data$intervals$lo)
parm_upper = as.list(Data$intervals$hi)
names(parm_lower) = Data$parm.names
names(parm_upper) = Data$parm.names
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot getting this note to fix. All these datasets refer to things in the ProSpectData package. Find all occurrences and fix. ❯ checking Rd cross-references ... NOTE |
Agent-Logs-Url: https://github.com/asgr/ProSpect/sessions/00d739b7-d48d-426a-ae77-0fcc76ea6e83 Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Fixed in b8d6867. I added explicit package anchors for all reported external Rd links, using |
ProSpectSEDlikewas still manually handling argument dispatch, bounds (lower/upper), and log-space transforms (logged), which diverged from currentParmOffcapabilities (including logical-vector.logged). This updates the likelihood path to useParmOffas the single parameter/dispatch mechanism with.check = FALSE.Parameter transform/bounds delegation
ParmOff::ParmOff(..., .return = "args", .check = FALSE)preprocessing step to produce the final scalar parameter vector used by the model call.Data$loggedto name-based selection so both scalar and logical-vector cases are handled consistently (includingzin photo-z mode).ProSpectSEDinvocation delegationdo.call("ProSpectSED", ...)call sites (returnall = TRUE/FALSE) withParmOff::ParmOff(.func = ProSpectSED, .return = "func", .check = FALSE).SFH,speclib,Dale,AGN,filtout,returnall,Data$arglist), while centralizing argument matching and filtering throughParmOff.Behavior-preserving guardrails
Data$constraintsapplication order before bounds/log transforms.scat_scale, likelihood, monitor, and return-shape behavior intact.ParmOffarg extraction.