Simplify profitLikeModel using ParmOff for argument matching, interval clamping, and log-unlogging#17
Conversation
Agent-Logs-Url: https://github.com/ICRAR/ProFit/sessions/af38ea0f-5efe-46d5-bc04-bbc3cd502caa Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Reviewer's GuideRefactors profitLikeModel evaluation to use the ParmOff package for argument matching and preprocessing when calling profitMakeModel and ProSpect::ProSpectSED, consolidating duplicated code paths, simplifying ProSpect SED handling, and adding a configurable scatter scaling parameter for likelihood calculations. Sequence diagram for ParmOff-mediated ProSpectSED evaluation in profitLikeModelsequenceDiagram
participant profitLikeModel
participant ParmOff
participant ProSpectSED as ProSpect::ProSpectSED
participant Jansky2magAB as ProSpect::Jansky2magAB
profitLikeModel->>profitLikeModel: build args_list, lower_i, upper_i, logged_i
profitLikeModel->>ParmOff: ParmOff(ProSpect::ProSpectSED, .args=args_list, .strip=_i, .lower=lower_i, .upper=upper_i, .logged=logged_i, .check=FALSE, returnall=FALSE)
ParmOff->>ProSpectSED: ProSpect::ProSpectSED(filtered_args)
ProSpectSED-->>ParmOff: sed_flux
ParmOff-->>profitLikeModel: sed_flux
profitLikeModel->>Jansky2magAB: ProSpect::Jansky2magAB(sed_flux)
Jansky2magAB-->>profitLikeModel: outSED (magnitudes)
profitLikeModel->>profitLikeModel: update Data[[j]]$modellist[[1]]$mag[i]
Sequence diagram for ParmOff-mediated profitMakeModel call in .profitLikeModelEvaluationsequenceDiagram
participant profitLikeModel
participant profitLikeModelEval as .profitLikeModelEvaluation
participant ParmOff
participant profitMakeModel
profitLikeModel->>profitLikeModelEval: .profitLikeModelEvaluation(Data, modellistnew,...)
profitLikeModelEval->>profitLikeModelEval: makemodel_args = list(..., adjust_calcregion=FALSE)
profitLikeModelEval->>profitLikeModelEval: [if Data$usecalcregion] add calcregion, docalcregion
profitLikeModelEval->>ParmOff: ParmOff(profitMakeModel, .args=makemodel_args, .check=FALSE, ...)
ParmOff->>profitMakeModel: profitMakeModel(matched_args)
profitMakeModel-->>ParmOff: model
ParmOff-->>profitLikeModelEval: model
profitLikeModelEval-->>profitLikeModel: model
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@copilot logging and intervals can be dealt with via ParmOff (https://github.com/asgr/ParmOff/blob/main/R/ParmOff.R) rewrite relevant profitLikeModel code to make use of that capability, ideally simplifying the code within profitLikeModel. |
…ging Agent-Logs-Url: https://github.com/ICRAR/ProFit/sessions/89f423ac-37f0-4d52-bd2a-dfcd8a545fec Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Done in 16b7e96. The pre-loop
ParmOff then applies clamping ( |
|
@copilot Given what ParmOff can now do (https://github.com/asgr/ParmOff) with functional parameter passing, are there any other simplifications or improvements that can be made in ProSpectSEDlike. |
Since 1.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the single-image code path (non-MultiBand/MultiImage),
scat_scaleis now used in thednormcall but is only defined inside the earlierinherits(Data, 'list') & inherits(Data[[1]], 'profit.data')branch, soscat_scalewill be undefined for standardprofit.datainputs; you likely want to factor thescat_scaleextraction out so it runs for both paths. - Given that
ParmOffis called with.check = FALSEin performance-critical paths, consider whether there should be a way (e.g. via a flag inData) to enable checks during debugging or when integrating new parameters, as silent mismatches or dropped arguments could otherwise be harder to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the single-image code path (non-MultiBand/MultiImage), `scat_scale` is now used in the `dnorm` call but is only defined inside the earlier `inherits(Data, 'list') & inherits(Data[[1]], 'profit.data')` branch, so `scat_scale` will be undefined for standard `profit.data` inputs; you likely want to factor the `scat_scale` extraction out so it runs for both paths.
- Given that `ParmOff` is called with `.check = FALSE` in performance-critical paths, consider whether there should be a way (e.g. via a flag in `Data`) to enable checks during debugging or when integrating new parameters, as silent mismatches or dropped arguments could otherwise be harder to diagnose.
## Individual Comments
### Comment 1
<location path="R/profitLikeModel.R" line_range="117-129" />
<code_context>
+ }
+
+ # Filter bounds and logged names to this component, strip _i suffix for ParmOff
+ strip_i = paste0('_', i)
+ lower_i = NULL
+ upper_i = NULL
+ if(!is.null(lower_ProSpect)) {
+ sel = grepl(strip_i, names(lower_ProSpect))
+ if(any(sel)) {
+ stripped_names = sub(strip_i, '', names(lower_ProSpect)[sel])
+ lower_i = setNames(lower_ProSpect[sel], stripped_names)
+ upper_i = setNames(upper_ProSpect[sel], stripped_names)
}
}
- outSED = ProSpect::Jansky2magAB(do.call(ProSpect::ProSpectSED, c(args_list, returnall=FALSE), quote=TRUE))
+ logged_i = sub(strip_i, '', all_logged_names[grepl(strip_i, all_logged_names)])
+
+ outSED = ProSpect::Jansky2magAB(ParmOff(ProSpect::ProSpectSED,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Component selection via `grepl(strip_i, ...)` may unintentionally match parameter names containing `_<i>` in non-suffix positions.
Here `grepl(paste0('_', i), param_name)` will also match `_<i>` anywhere in the name. If any ProSpect parameters contain `"_1"`, `"_2"`, etc. in the middle of the name, they’ll be treated as component-specific, have that substring stripped, and may end up with incorrect names and lower/upper/logged metadata. If the convention guarantees `_<i>` only as a terminal suffix this is safe; otherwise, consider anchoring to the end (e.g. `grepl(paste0('_', i, '$'), ...)`) to avoid accidental matches.
```suggestion
# Filter bounds and logged names to this component, strip _i suffix for ParmOff
strip_i = paste0('_', i)
strip_pattern = paste0(strip_i, '$')
lower_i = NULL
upper_i = NULL
if(!is.null(lower_ProSpect)) {
sel = grepl(strip_pattern, names(lower_ProSpect))
if(any(sel)) {
stripped_names = sub(strip_pattern, '', names(lower_ProSpect)[sel])
lower_i = setNames(lower_ProSpect[sel], stripped_names)
upper_i = setNames(upper_ProSpect[sel], stripped_names)
}
}
logged_i = sub(strip_pattern, '', all_logged_names[grepl(strip_pattern, all_logged_names)])
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Filter bounds and logged names to this component, strip _i suffix for ParmOff | ||
| strip_i = paste0('_', i) | ||
| lower_i = NULL | ||
| upper_i = NULL | ||
| if(!is.null(lower_ProSpect)) { | ||
| sel = grepl(strip_i, names(lower_ProSpect)) | ||
| if(any(sel)) { | ||
| stripped_names = sub(strip_i, '', names(lower_ProSpect)[sel]) | ||
| lower_i = setNames(lower_ProSpect[sel], stripped_names) | ||
| upper_i = setNames(upper_ProSpect[sel], stripped_names) | ||
| } | ||
| } | ||
| outSED = ProSpect::Jansky2magAB(do.call(ProSpect::ProSpectSED, c(args_list, returnall=FALSE), quote=TRUE)) | ||
| logged_i = sub(strip_i, '', all_logged_names[grepl(strip_i, all_logged_names)]) |
There was a problem hiding this comment.
suggestion (bug_risk): Component selection via grepl(strip_i, ...) may unintentionally match parameter names containing _<i> in non-suffix positions.
Here grepl(paste0('_', i), param_name) will also match _<i> anywhere in the name. If any ProSpect parameters contain "_1", "_2", etc. in the middle of the name, they’ll be treated as component-specific, have that substring stripped, and may end up with incorrect names and lower/upper/logged metadata. If the convention guarantees _<i> only as a terminal suffix this is safe; otherwise, consider anchoring to the end (e.g. grepl(paste0('_', i, '$'), ...)) to avoid accidental matches.
| # Filter bounds and logged names to this component, strip _i suffix for ParmOff | |
| strip_i = paste0('_', i) | |
| lower_i = NULL | |
| upper_i = NULL | |
| if(!is.null(lower_ProSpect)) { | |
| sel = grepl(strip_i, names(lower_ProSpect)) | |
| if(any(sel)) { | |
| stripped_names = sub(strip_i, '', names(lower_ProSpect)[sel]) | |
| lower_i = setNames(lower_ProSpect[sel], stripped_names) | |
| upper_i = setNames(upper_ProSpect[sel], stripped_names) | |
| } | |
| } | |
| outSED = ProSpect::Jansky2magAB(do.call(ProSpect::ProSpectSED, c(args_list, returnall=FALSE), quote=TRUE)) | |
| logged_i = sub(strip_i, '', all_logged_names[grepl(strip_i, all_logged_names)]) | |
| # Filter bounds and logged names to this component, strip _i suffix for ParmOff | |
| strip_i = paste0('_', i) | |
| strip_pattern = paste0(strip_i, '$') | |
| lower_i = NULL | |
| upper_i = NULL | |
| if(!is.null(lower_ProSpect)) { | |
| sel = grepl(strip_pattern, names(lower_ProSpect)) | |
| if(any(sel)) { | |
| stripped_names = sub(strip_pattern, '', names(lower_ProSpect)[sel]) | |
| lower_i = setNames(lower_ProSpect[sel], stripped_names) | |
| upper_i = setNames(upper_ProSpect[sel], stripped_names) | |
| } | |
| } | |
| logged_i = sub(strip_pattern, '', all_logged_names[grepl(strip_pattern, all_logged_names)]) |
There was a problem hiding this comment.
Pull request overview
Refactors profitLikeModel’s core evaluation path to delegate argument matching and parameter preprocessing (suffix stripping, interval clamping, log→linear transforms) to ParmOff, reducing duplicated/manual do.call and filtering logic in both model construction and the ProSpect SED component loop.
Changes:
- Replaced duplicated
profitMakeModelcall branches with a singleParmOff(profitMakeModel, .args=...)invocation. - Simplified the ProSpect SED loop by using
ParmOff(ProSpect::ProSpectSED, .strip/.lower/.upper/.logged=...)instead of manual suffix stripping, bounds clamping, and unlogging. - Added
ParmOffas an imported dependency and declared its remote.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
R/profitLikeModel.R |
Refactors model evaluation + ProSpect SED argument handling to use ParmOff; introduces scat_scale usage in normal likelihood paths. |
NAMESPACE |
Imports ParmOff::ParmOff for unqualified use in R code. |
DESCRIPTION |
Adds ParmOff to Imports and asgr/ParmOff to Remotes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| skewtparm = Data$skewtparm | ||
|
|
||
| if(isnorm){ | ||
| LL=sum(dnorm(cutsig, log=TRUE)) | ||
| LL=sum(dnorm(cutsig, mean=0, sd=scat_scale, log=TRUE)) | ||
| } else if(ischisq) { |
| if('scat_scale' %in% Data$parm.names){ | ||
| sel = which(Data$parm.names == 'scat_scale') | ||
| scat_scale = parm[sel] | ||
| parm = parm[-sel] | ||
| Data$parm.names = Data$parm.names[-sel] | ||
| }else if('log_scat_scale' %in% Data$parm.names){ | ||
| sel = which(Data$parm.names == 'log_scat_scale') | ||
| scat_scale = 10^parm[sel] | ||
| parm = parm[-sel] | ||
| Data$parm.names = Data$parm.names[-sel] | ||
| }else{ | ||
| scat_scale = 1 | ||
| } |
Agent-Logs-Url: https://github.com/ICRAR/ProFit/sessions/0e0b7f90-f28f-437e-a9e5-a7d01f46ec3f Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Implemented in commit 16b7e96: the ProSpect loop now uses
I made one more simplification in this PR in commit 2feed91: |
|
@copilot Given what ParmOff can now do (https://github.com/asgr/ParmOff) with functional parameter passing, are there any other simplifications or improvements that can be made in profitLikeModel. |
Agent-Logs-Url: https://github.com/ICRAR/ProFit/sessions/69adf167-357e-4a31-b8d9-ddb5b06abbab Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Yes—implemented one additional improvement in |
Summary
Refactors
profitLikeModel.Rto use theParmOffpackage for function argument matching, eliminating manual arg filtering, suffix stripping, interval clamping, log-unlogging, anddo.callboilerplate.Changes
DESCRIPTION/NAMESPACEParmOfftoImports(required at load time since it's used in the core evaluation path)asgr/ParmOfftoRemotesimportFrom("ParmOff", "ParmOff")toNAMESPACE.profitLikeModelEvaluation— merged duplicateprofitMakeModelcallsThe old code had two near-identical
profitMakeModelcalls (one withcalcregion/docalcregion, one without), branched onData$usecalcregion. Replaced with a singleParmOff(profitMakeModel, .args=makemodel_args, ...)call where the calcregion args are conditionally added to the args list beforehand.ProSpect SED loop — simplified arg stripping, data merging, interval clamping, and log-unlogging
The old
for(i in 1:Data$Ncomp)loop did:sub()/names()to strip_isuffix from param namesif(Ncomp==1)vs multi-component) to mergedata_ProSpect, withgrepl/data_loc_globalfilteringpmin/pmaxblock to clamp parameters viaData$intervals_ProSpect10^parmunlogging block driven byData$logged_ProSpect, with branching for single-logical vs logical-vector casesparm[args_loc] = parm_loggedwrite-back to propagate processed values into the loopAll of this is replaced by passing
.strip,.lower,.upper, and.loggedto a singleParmOff(ProSpect::ProSpectSED, ...)call per component:.striphandles suffix removal automatically.lower/.upper(named lists built fromData$intervals_ProSpect$lo/hi) handle interval clamping.logged(character vector of param names derived fromData$logged_ProSpect) handles10^xunloggingdata_ProSpectentries, eliminating theif(Ncomp==1)branch entirelyparmwrite-back are removedSummary by Sourcery
Refactor profitLikeModel to delegate argument handling and ProSpect SED evaluation to ParmOff while adding configurable scatter scaling to normal likelihood calculations.
New Features:
Enhancements:
Build: