Add covariance penalty option and configurable logging of sale price by wagnerlmichael · Pull Request #475 · ccao-data/model-res-avm

wagnerlmichael · 2026-04-08T14:28:44Z

The main contribution of this PR is to wire in logic to take advantage of lightgbm's support of custom objective functions to add the custom objective function option that the MIT research team has developed with the aim of improving vertical equity. Based on our own internal investigation, it seems to be quite a promising specification. There is EDA that show specific figures in the EI 408 output folder.

This PR adds params.yaml file edits which include

The ability to select the custom obj function from params.yaml (mse_cov)
A toggle to set the model to log the training data. This is necessary for our implementation of the custom objective function. Adding the ability to log the outcome variable is also a generally good option to have in terms of ML modeling strategies.

With select * from model.metadata where run_id = '2026-05-20-infallible-dan' you can see the persistence of log_transform_enable

With SELECT * FROM "model"."parameter_final" where run_id = '2026-05-20-infallible-dan' you can see objective = mse_cov and mse_cov_rho=1

Supported by lightbgm PR: ccao-data/lightsnip#21

Other approaches tried

We tried using tailor and building the transforms into the tidymodels workflow
model res - #520.

We also tried smuggling the transforms into lightsnip itself

model res PR: lightsnip smuggle [DRAFT] #524
lightsnip PR: lightsnip smuggle [DRAFT] lightsnip#27

Ultimately we decided against these other strategies due to a combination of increased complexity and recommendations against doing these types of transforms from the tidymodels maintainers.

Merge in main

wagnerlmichael · 2026-04-21T17:38:13Z

@jeancochrane Would love your feedback re:

If we were to merge this, we would likely need to:

Build in a boolean switch to params.yaml which allows to toggle logging price on and off

Decide on a strategy for persisting the tuning variable (in this case rho), to the output tables in S3 (in pipline/05-finalize.R). One idea is to add a variable which grabs params$objective$custom_obj_tuning_variable and instantiate it as something flexible like a json obj "{rho: 3}"

Currently the logging of price (and exponentiating) is hard-coded, I'm thinking I can just build in something like log_outcome_before_training boolean. This could be uploaded as a new column in model.parameter_final uploaded in 05-finalize.R.

What I'm imagining as the more complicated piece is persisting the custom obj function metadata. The model.parameter_final.objective should work well with how we have it defined ("mse_cov") currently in this diff, but we currently don't have a space for a tuning variable like mse_cov_rho (also in this diff).

I'm trying to think of a solution here that would work for other custom obj functions in the future that could have any number of additional parameters to tune.

Currently the mse_cov_rho is only recruited when we set the objective to a "mse_cov", otherwise it is ignored. This feels a little clunky but was the quickest easiest way to test. If we were to have multiple custom obj functions using this strategy, we would either need to upload all of these keys, or build in some sort of logic to upload whichever custom parameters are associated with the objective function used in the model run.

A low effort solution could be to dump them all in a json obj in a new column, but I think there is probably something better. We could also start a condition in the code like if this objective function, then these custom tuning variables name -> {"rho": 3}, which would just be incremented with each new addition.

What do you think?

Tagging @TimCookCountyDS, to make sure my understanding of potential future custom objective functions lines up with his, and for visibility

jeancochrane · 2026-04-21T19:08:58Z

Currently the logging of price (and exponentiating) is hard-coded, I'm thinking I can just build in something like log_outcome_before_training boolean. This could be uploaded as a new column in model.parameter_final uploaded in 05-finalize.R.

Sounds great to me!

What I'm imagining as the more complicated piece is persisting the custom obj function metadata. The model.parameter_final.objective should work well with how we have it defined ("mse_cov") currently in this diff, but we currently don't have a space for a tuning variable like mse_cov_rho (also in this diff). [...] A low effort solution could be to dump them all in a json obj in a new column, but I think there is probably something better. We could also start a condition in the code like if this objective function, then these custom tuning variables name -> {"rho": 3}, which would just be incremented with each new addition.

This is a good concern to raise. I wonder whether we are likely to add lots of new objective functions with their own specific parameters, and if so, how many (ballpark) we expect to add? If we think we will add lots of new objective functions in the future (e.g. more than, say, 5) then I think the flexible JSON-based schema you suggest may be worth it. If we only expect a small set of new objective functions, however, then I think the additional complexity of mixing JSON fields with regular fields won't be worth it.

wagnerlmichael · 2026-05-18T19:30:40Z

Currently the logging of price (and exponentiating) is hard-coded, I'm thinking I can just build in something like log_outcome_before_training boolean. This could be uploaded as a new column in model.parameter_final uploaded in 05-finalize.R.

Sounds great to me!

What I'm imagining as the more complicated piece is persisting the custom obj function metadata. The model.parameter_final.objective should work well with how we have it defined ("mse_cov") currently in this diff, but we currently don't have a space for a tuning variable like mse_cov_rho (also in this diff). [...] A low effort solution could be to dump them all in a json obj in a new column, but I think there is probably something better. We could also start a condition in the code like if this objective function, then these custom tuning variables name -> {"rho": 3}, which would just be incremented with each new addition.

This is a good concern to raise. I wonder whether we are likely to add lots of new objective functions with their own specific parameters, and if so, how many (ballpark) we expect to add? If we think we will add lots of new objective functions in the future (e.g. more than, say, 5) then I think the flexible JSON-based schema you suggest may be worth it. If we only expect a small set of new objective functions, however, then I think the additional complexity of mixing JSON fields with regular fields won't be worth it.

Turns out due to how the pipeline right parameters we already have mse_cov_rho saved in model.parameter_final.
SELECT mse_cov_rho FROM "model"."parameter_final" where run_id = '2026-04-11-magical-manasi'

I think for now it may make sense to build it without an extensible version in mind, as I don't think we will be adding more objective functions any time soon. But I could be wrong, I suspect a migration given the future need for it would be relatively painless. Let me know what you think @TimCookCountyDS @jeancochrane

TimCookCountyDS · 2026-05-18T19:43:30Z

Turns out due to how the pipeline right parameters we already have mse_cov_rho saved in model.parameter_final. SELECT mse_cov_rho FROM "model"."parameter_final" where run_id = '2026-04-11-magical-manasi'

I think for now it may make sense to build it without an extensible version in mind, as I don't think we will be adding more objective functions any time soon. But I could be wrong, I suspect a migration given the future need for it would be relatively painless. Let me know what you think @TimCookCountyDS @jeancochrane

I think if the objective function is currently captured in the model.parameter final table we should be ok for now? I think the only additional function we were considering adding was the smooth penalty (which iirc had some tighter theoretical properties?), but even that is entirely optional -

(That said, the json schema is good to keep in mind should things get more complicated).

wagnerlmichael · 2026-05-26T09:43:01Z

-      "RemoteRef": "master",
-      "RemoteSha": "2720ad973c990ad64ac7e6bb632389ff9f2a2675",
+      "RemoteRef": "test-new-obj-function",
+      "RemoteSha": "f228237f4595cb15c8ff5482d06d116595e4ec16",


Reminder to revert this to master after merging in lightsnip PR

wagnerlmichael · 2026-05-26T11:33:55Z

+    if (log_transform_enable) {
+      preds <- preds %>%
+        mutate(
+          pred_card_initial_fmv = exp(pred_card_initial_fmv),


Revert log scale for interpretable results and downstream values

wagnerlmichael · 2026-05-26T12:43:13Z

 # Note included with each run. Use this to summarize what changed about the run
 # or add context
-run_note: Potential residential baseline with SHAP values, revert DVC path
+run_note: Test cov term    


Should I leave this as it was? Does it also make sense to revert the log_transform_enable and the objective to the prior state here? For me I don't have a good sense of which things we should keep stable on the master branch of our models.
I also haven't yet edited the dvc.lock file, should I go ahead dvc repro to do that?

I can run repro on the server tomorrow (I won't be taking up resources at the same time as anyone else), or we would take it as an opportunity to see if someone else can replicate these changes/a model run.

[Suggestion, non-blocking] Personally, I think we should leave run_type and run_note set to the final values for the 2026 model until we're ready to start next year's modeling session. It's a purely aesthetic preference, however, so I'm fine changing them now if other folks would prefer to do that.

I'll follow Jean's lead here - and will add - as long as we're saving which objective function we use, and the log transform in the params- (so that we can track experiments/tests) - I don't have strong feelings on run_type/run_note here.

jeancochrane

Great start here! I have a few big-picture questions about the design, happy to chat them out if it would be helpful.

jeancochrane · 2026-05-27T16:35:23Z

 # Note included with each run. Use this to summarize what changed about the run
 # or add context
-run_note: Potential residential baseline with SHAP values, revert DVC path
+run_note: Test cov term    


[Suggestion, non-blocking] Personally, I think we should leave run_type and run_note set to the final values for the 2026 model until we're ready to start next year's modeling session. It's a purely aesthetic preference, however, so I'm fine changing them now if other folks would prefer to do that.

jeancochrane · 2026-05-27T16:40:39Z

+  # Log-transform `meta_sale_price` for LightGBM; predictions are
+  # exp()-scaled back. Linear baseline always uses log(price).
+  log_transform_enable: true


[Thought, non-blocking] I'm wondering if it makes sense for this param to be a toggle, or if we should move it somewhere else in this file? All of the other toggles are implemented that way because they are features that we frequently want to switch on and off during the course of normal model development and operation. The log transformation strikes me as being more similar to a parameter like model.objective in that we will likely choose one approach that works best for us and stick with it. As evidence of this likelihood, it doesn't seem like we plan to expose a workflow option for log_transform_enable, which is the primary interface through which we use toggles. What do you think about moving this param to something like model.log_sale_price instead? I'm curious what other reviewers think, too.

" As evidence of this likelihood, it doesn't seem like we plan to expose a workflow option for log_transform_enable" - good point :)

"The log transformation strikes me as being more similar to a parameter like model.objective in that we will likely choose one approach that works best for us and stick with it." <- this tracks for me : I imagine we'll want to be able to change it from time to time, based on new data, but throwing it under the model heading works for me.

Great point on the workflow toggle, I've changed the param to model.log_sale_price, the references in the pipeline are still log_transform_enable, I think it makes sense to switch those to log_sale_price as well starting in setup.R, then after finalizing, delete the testing runs and re-crawl (not sure if we actually need to re-crawl) to replace the column in our tables?

Does that sound right to you?

…vm into test-log-transform Merge

wagnerlmichael · 2026-06-02T07:29:09Z

-    training_data_full
-  } else {
-    training_data_full %>% mutate(meta_sale_price = log(meta_sale_price))
-  },


Factored this out into recipes because if we were moving the lgbm log config there, it seemed like it made sense for this to go there too

wagnerlmichael · 2026-06-02T07:34:30Z

+  # therefore come back on the log scale -- see to_dollars() for the inverse
+  if (log_outcome) {
+    main_recipe <- main_recipe %>%
+      step_log(meta_sale_price, skip = TRUE)


I don't think there is an easy way to build in exponentiation at predict time into the bake/predict workflow, but step_log is supported for the training with the skip arg. Not sure if you had other ideas

[Suggestion, optional] Hmm, it would definitely be a bummer if we couldn't include the exponentiation step in the workflow. There will be a lot of room for error if we have to remember to call the to_dollars() or exp() functions on our predictions, especially if we have to call them differently for the tree vs. linear models.

I did a little digging and it seems like the tailor package is the tidymodels-supported way of adding postprocessing steps. It's still experimental but seems like the recommended way of solving this problem; see this blog post with a bit more background, and a demonstration of the way in which the workflows package has native support for tailor postprocessors via the add_tailor() function. I found the docs a bit confusing because the post-processing steps they include in their examples seem complex to me as a lay reader, but the adjust_predictions_custom() postprocessor seems like it would work out of the box with a simple exp() call like adjust_predictions_custom(.pred = exp(.pred)). Our proposed postprocessing step is a pure mathematical function, no learning required, so I don't think we need to worry about data leakage as described in the "Data-dependent transformations" and "Data Usage" sections of the docs.

Does this all make sense, and does it seem like a promising path forward to you too? If so, I say we go ahead and try it out before we decide to move forward with to_dollars()/exp(). I'm also happy to chat this out tomorrow if it would make things clearer.

This makes sense to me! I think the cost to remember to exp the outcome, if tied to a single param key, would be quite low, particularly because even if the exp gets called on non-logged data it will be extremely obvious looking at any sort of metric. Perhaps I'm missing something, though.

All of that to say I still think using a supported post-processing step makes much more sense from an organizational/readability/LoC perspective. Nice find, I'm going to dig in and see if I can work it in

Attempting to formalize ideas on potential problems with implementation of the tailor package as a log/exp solution

Hurdle 1 - serialization

Essentially the idea is that when the model gets saved, we don't currently persist post-processing information, so we would have to find a way to have the tailor exp transform persist through to 02-assess.r

Consider the model persist code in 01-train.R:

# Save the finalized model object to file so it can be used elsewhere. Note the # lgbm_save() function, which uses lgb.save() rather than saveRDS(), since # lightgbm is picky about how its model objects are stored on disk lgbm_wflow_final_full_fit %>% workflows::extract_fit_parsnip() %>% lightsnip::lgbm_save(paths$output$workflow_fit$local) # Save the finalized recipe object to file so it can be used to preprocess # new data. This is critical since it saves the factor levels used to integer- # encode any categorical columns lgbm_wflow_final_full_fit %>% workflows::extract_recipe() %>% lightsnip::axe_recipe() %>% saveRDS(paths$output$workflow_recipe$local)

In the first chunk, we only persist the model fit itself. In the second chunk, extract_recipe pulls from workflow$pre and the tailor code would live in workflow$post. I believe there is an extract_postprocessor() (shipped with the whole tailor ecosystem bump) function that could possibly be used for this use case, but then we are saving another object in 01-train.R and re-attaching in 02.asses.R, is that an option worth pursuing?

Hurdle 2 - existing CV problem with step_log()

With the addition of step_log the CV code is training in log space but doing per-model evaluation in dollar space.

In CV to score a fold, tune processes the assessment (held-out) rows through the recipe to get both predictors and the truth column, then hands (truth, estimate) to yardstick. That processing is a bake() call and because step_log has skip = TRUE, it is skipped here so the assessment meta_sale_price (the truth yardstick compares against) is left in raw dollars, while .pred is in log.

So even with the recent to_dollars proof-of-concept commit I introduced a high-impact CV bug.

General thoughts

My emerging opinion is that the whole recipe workflow works well enough for feature transformations, but for the target, sale price, it is clunkier and riskier to make changes to.

See in the (admittedly outdated) tidymodels book: https://www.tmwr.org/recipes

For simple transformations of the outcome column(s), we strongly suggest that those operations be conducted outside of the recipe.

Which makes the prior verbose and unclean if statements a more attractive option, though I'm sure those could be cleaned up in some way.

"My emerging opinion is that the whole recipe workflow works well enough for feature transformations, but for the target, sale price, it is clunkier and riskier to make changes to."

My thoughts (largely, at this moment, subjective, from past projects with tidymodels) generally align with this. Sometimes trying to shoe-horn things into a recipe leads to more complication than necessary.

re:
"Which makes the prior verbose and unclean if statements a more attractive option, though I'm sure those could be cleaned up in some way."

For our pipeline we have a minimum number of transforms to make, correct? (log transform the training inputs, exponentiate the predictions)- Is there any bigger concern about any processes that (later) use the model, or consume it's outputs?
(If so we may just want to flag the need to log transform/exponentiate as appropriate).

[Update]: working on Jean's custom post processor proposition

TimCookCountyDS · 2026-06-03T14:30:42Z

going to fire off a test on AWS

@TimCookCountyDS Can this wait until we get the param naming sorted out, so that we don't have to go in and clean up draft parameter names in the model.metadata table?

Sorry I missed this comment and had fired off a model run -goofy-tayun- - I didn't mean to create more work, apologies

This reverts commit eb935b0.

jeancochrane · 2026-07-01T20:12:25Z

A few thoughts for next steps:

Store logged sale prices and predictions in the training/assessment sets
Update model experiments doc as part of this PR so we can preserve our investigation (link to tailor PR)
Make a list of downstream consumers that need to change
Is it possible/simple to log/exponentiate in lightsnip? That might be better than tailor while still helping us simplify the train/predict interface

wagnerlmichael · 2026-07-09T18:46:45Z

      "RemoteRepo": "lightsnip",
-      "RemoteRef": "master",
-      "RemoteSha": "2720ad973c990ad64ac7e6bb632389ff9f2a2675",
+      "RemoteRef": "test-new-obj-function",


Todo: Switch this before merging

jeancochrane

Awesome work! I'm sorry I led us on a wild goose chase to try to simplify this... reviewing it again, I'm seeing that it's actually not that bad 😅

I do think we should preserve these logged outcomes, but I'm happy to save that for a follow-up PR, so I opened up #528 to track the task.

jeancochrane

Looks great!

…vm into test-log-transform Merge conflict fix

wagnerlmichael added 4 commits March 31, 2026 14:08

Add log changes

a456a9c

Merge branch 'master' into test-log-transform

d84f60d

Merge in main

Add custom obj function

1ce5727

Point to lightsnip branch

0bd576d

wagnerlmichael temporarily deployed to deploy April 10, 2026 18:05 — with GitHub Actions Inactive

wagnerlmichael temporarily deployed to deploy April 10, 2026 18:40 — with GitHub Actions Inactive

wagnerlmichael changed the title ~~[WIP, EDA] Test log transform~~ [WIP, EDA] Test log transform and cov penalty Apr 13, 2026

wagnerlmichael mentioned this pull request Apr 20, 2026

Add MSE + covariance penalty term custom objective function ccao-data/lightsnip#21

Merged

Add log toggle

a3f14fa

wagnerlmichael added 2 commits May 18, 2026 14:50

Add boolean log toggle to metadata file

a2e40dd

Update params

c7cb733

wagnerlmichael temporarily deployed to deploy May 19, 2026 14:30 — with GitHub Actions Inactive

Edit comment

33e2dd3

wagnerlmichael changed the title ~~[WIP, EDA] Test log transform and cov penalty~~ [WIP, EDA] Add covariance penalty option and configurable logging of sale price May 26, 2026

wagnerlmichael commented May 26, 2026

View reviewed changes

Fix merge conflict and merge master

fc26567

wagnerlmichael commented May 26, 2026

View reviewed changes

Merge branch 'master' into test-log-transform

82e33d1

wagnerlmichael commented May 26, 2026

View reviewed changes

wagnerlmichael marked this pull request as ready for review May 26, 2026 12:50

wagnerlmichael requested review from TimCookCountyDS, jeancochrane and wrridgeway as code owners May 26, 2026 12:50

wagnerlmichael changed the title ~~[WIP, EDA] Add covariance penalty option and configurable logging of sale price~~ Add covariance penalty option and configurable logging of sale price May 26, 2026

Merge branch 'master' into test-log-transform

d832d39

jeancochrane reviewed May 27, 2026

View reviewed changes

wagnerlmichael added 4 commits May 28, 2026 01:13

Merge branch 'test-log-transform' of github.com:ccao-data/model-res-a…

180a036

…vm into test-log-transform Merge

Rework log toggle organization

e00ee73

Remove unecessary comment

19ebe9c

Add log and exp re-structure

eb935b0

wagnerlmichael commented Jun 2, 2026

View reviewed changes

wagnerlmichael mentioned this pull request Jun 5, 2026

Add cov pen and log toggle #512

Closed

wagnerlmichael mentioned this pull request Jun 26, 2026

Tailor post processing transform proof of concept #520

Closed

Revert "Add log and exp re-structure"

dd9685c

This reverts commit eb935b0.

ccao-jardine mentioned this pull request Jun 30, 2026

Offboard Tim Sparer ccao-data/people#40

Closed

6 tasks

jeancochrane mentioned this pull request Jun 30, 2026

MIT regressivity penalty implementation: R lgbm modifications, Finalize the addition of the basic covariance penalty to lightsnip + lgbm and test in pipeline #480

Closed

wagnerlmichael added 2 commits July 9, 2026 10:59

Rvert param defaults to master

3b7a394

Revert run_type

7a8be66

wagnerlmichael commented Jul 9, 2026

View reviewed changes

jeancochrane mentioned this pull request Jul 13, 2026

Preserve logged sale prices and predictions #528

Open

jeancochrane approved these changes Jul 13, 2026

View reviewed changes

Comment thread params.yaml Outdated

Comment thread params.yaml Outdated

Add mse_cov and log switch check

3198830

jeancochrane approved these changes Jul 14, 2026

View reviewed changes

Comment thread R/setup.R Outdated

wagnerlmichael and others added 8 commits July 14, 2026 12:24

Switch objective to mse_cov

8a46317

Format to remove nolint

9eaf80e

Merge branch 'master' into test-log-transform

6d2ab97

Point to lightsnip master

97557d8

Merge branch 'test-log-transform' of github.com:ccao-data/model-res-a…

22e8432

…vm into test-log-transform Merge conflict fix

Swap log training to true

9b1ef34

Add dvc.lock diff after local run

ad83cf8

Merge branch 'master' into test-log-transform

398bafd

wagnerlmichael merged commit a48f0bf into master Jul 15, 2026
9 checks passed

wagnerlmichael deleted the test-log-transform branch July 15, 2026 15:30

Uh oh!

Conversation

wagnerlmichael commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other approaches tried

Uh oh!

wagnerlmichael commented Apr 21, 2026

Uh oh!

jeancochrane commented Apr 21, 2026

Uh oh!

wagnerlmichael commented May 18, 2026

Uh oh!

TimCookCountyDS commented May 18, 2026

Uh oh!

wagnerlmichael May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wagnerlmichael May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeancochrane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wagnerlmichael Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wagnerlmichael Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeancochrane Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Hurdle 1 - serialization

Hurdle 2 - existing CV problem with step_log()

General thoughts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimCookCountyDS commented Jun 3, 2026

Uh oh!

jeancochrane commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeancochrane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jeancochrane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wagnerlmichael commented Apr 8, 2026 •

edited

Loading

wagnerlmichael May 26, 2026 •

edited

Loading

wagnerlmichael May 26, 2026 •

edited

Loading

wagnerlmichael Jun 1, 2026 •

edited

Loading

wagnerlmichael Jun 2, 2026 •

edited

Loading

jeancochrane Jun 2, 2026 •

edited

Loading

Hurdle 2 - existing CV problem with `step_log()`

jeancochrane commented Jul 1, 2026 •

edited

Loading