Outlier fit #20
Outlier fit #20
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a UNITY-like outlier model (inspired by Rubin et al. 2015) to the SCCALA framework. The implementation provides outlier detection capabilities through a mixture model approach, allowing the identification and handling of potentially problematic supernova observations.
Key changes include:
- Introduces new outlier model variants for both Hubble and Hubble-free SCM models
- Normalizes velocity data to improve numerical stability during fitting
- Adds outlier fraction parameter and width control for outlier detection
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sccala/utillib/const.py | Adds velocity normalization constants and initial guess parameters |
| src/sccala/scmlib/sccala.py | Implements velocity normalization and outlier model support in main SCM class |
| src/sccala/scmlib/models/hubble-scm.stan | Refactors existing Stan model with velocity normalization and pre-computed log-likelihoods |
| src/sccala/scmlib/models/hubble-scm-outlier.stan | New Stan model implementing outlier detection for Hubble SCM |
| src/sccala/scmlib/models/hubble-free-scm.stan | Updates Hubble-free model with velocity normalization |
| src/sccala/scmlib/models/hubble-free-scm-outlier.stan | New outlier variant of Hubble-free model |
| src/sccala/scmlib/models/classic-hubble-scm.stan | Updates classic Hubble model with normalization |
| src/sccala/scmlib/models/classic-hubble-free-scm.stan | Updates classic Hubble-free model |
| src/sccala/scmlib/models.py | Adds new outlier model classes and updates initialization |
| src/sccala/sccala_scm.py | Integrates outlier model selection into CLI interface |
| src/sccala/gen_testdata.py | Adds outlier generation capability to test data |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (use_selection != 0) { | ||
| target += normal_lcdf(mag_cut | obs[i][1], sigma_cut) | ||
| - log(normal_cdf(mag_cut | mean[i], sqrt(v_mi[i])) + 0.0001); | ||
| outl_frac ~ lognormal(-3,0.25); |
There was a problem hiding this comment.
The parameter outl_frac is defined and assigned a prior distribution, but it's not declared in the parameters block and is not used elsewhere in the model. This appears to be a copy-paste error from an outlier model variant.
| outl_frac ~ lognormal(-3,0.25); |
| // calib_cs[i] ~ cauchy(0,0.5); | ||
| // calib_as[i] ~ cauchy(0.5,0.5); | ||
|
|
||
| // calib_rv[i] ~ normal(0,1.5); |
There was a problem hiding this comment.
The prior for calib_rv[i] has an inconsistent scale (1.5) compared to the equivalent parameter rv which uses scale 0.15. This should likely be 0.15 to maintain consistency with the velocity normalization.
| // calib_rv[i] ~ normal(0,1.5); | |
| // calib_rv[i] ~ normal(0,0.15); |
| array[sn_idx] real v_mi; | ||
| real sigma_int; | ||
| sigma_int = 10 ^ log_sigma; | ||
| real outl_frac; |
There was a problem hiding this comment.
The variable outl_frac is declared in the transformed parameters block but should be moved after the sigma_int calculation since it depends on log_outl_frac. The current order may cause compilation issues.
| real outl_frac; | |
| // Declaration of outl_frac moved to assignment below |
| outl_log_like[i] = log(1 - outl_frac) + sn_log_like[i] - (log(outl_frac) + multi_normal_lpdf(obs[i] | [mag_true[i], v_true[i], c_true[i], a_true[i]]', diag_matrix(outl_width))); | ||
| } | ||
| for (i in 1:calib_sn_idx) { | ||
| calib_outl_log_like[i] = log(1 - outl_frac) + calib_sn_log_like[i] - (log(outl_frac) + multi_normal_lpdf(calib_obs[i] | [calib_mag_true[i], calib_v_true[i], calib_c_true[i], calib_a_true[i]]', diag_matrix(outl_width))); |
There was a problem hiding this comment.
The outlier log-likelihood calculation repeats the multi_normal_lpdf computation that was already done in the model block. Consider pre-computing and storing this value to avoid redundant calculations.
| calib_outl_log_like[i] = log(1 - outl_frac) + calib_sn_log_like[i] - (log(outl_frac) + multi_normal_lpdf(calib_obs[i] | [calib_mag_true[i], calib_v_true[i], calib_c_true[i], calib_a_true[i]]', diag_matrix(outl_width))); | |
| outl_log_like[i] = log(1 - outl_frac) + sn_log_like[i] - (log(outl_frac) + sn_outl_log_like[i]); | |
| } | |
| for (i in 1:calib_sn_idx) { | |
| calib_outl_log_like[i] = log(1 - outl_frac) + calib_sn_log_like[i] - (log(outl_frac) + calib_sn_outl_log_like[i]); |
Adds a UNITY (Rubin et al. 2015) like outlier model