Include imputed strata in assessment card/pin #55

Damonamajor · 2024-11-18T15:18:44Z

During this PR, it was made known that the step_impute_knn is not deterministic and thus different model runs will have different results. This meant we add documentation to the Recipes.R file.
Alongside this, we extract values from the bake stage of the condo pipeline and revert them to their original values (adding a meta_strata_is_imputed binary variable.

Damonamajor · 2024-11-18T15:21:22Z

pipeline/02-assess.R


+mapping_1 <- assessment_data_pred %>%


I think this can be minorly optimized by use deframe from the tibble package, but that's not part of the existing renv.

Damonamajor · 2024-11-18T15:30:23Z

pipeline/02-assess.R

@@ -268,7 +296,8 @@ assessment_data_pin <- assessment_data_merged %>%
    meta_year, meta_pin, meta_pin10, meta_triad_code, meta_township_code,
    meta_nbhd_code, meta_tax_code, meta_class, meta_tieback_key_pin,
    meta_tieback_proration_rate, meta_cdu, meta_modeling_group,


Is there any documentation that needs to be updated with new values in pin / card output files? Strata was never in the pin output file to begin with.

Nope! This should just get automatically added to the equivalent tables in Athena once it's crawled.

dfsnow

@Damonamajor This is mostly looking great! A few minor cleanups in the comments. Can you also make a new issue to add the meta_strata values and flag (is_imputed) to the Desk Review spreadsheets (so edit 07-export.R).

dfsnow · 2024-11-18T18:13:42Z

R/recipes.R

+    # impute missing condo building strata. Within step_impute_knn, an estimated
+    # node value is called with the sample(). This is not deterministic, meaning
+    # different runs of the model will have different imputed values, and thus
+    # different FMVs.


issue (blocking): This is sort of correct. The imputation will be deterministic between model runs if a seed is set somewhere in this file. However, it won't be deterministic if you run the same prediction twice in a single session (unless you set the seed again).

I would add set.seed(params$input$strata$seed) somewhere at the top of this file to ensure that prediction is always using the same seed. Then run the stage twice (run once, restart, run again) and check that the results are the same.

I included this in the file as described, and as setting a global seed. In each of those situations, different iterations still created different FMVs. You can use the uploaded testing file to look into this.

dfsnow · 2024-11-18T18:27:07Z

pipeline/02-assess.R

+mapping_2 <- assessment_data_pred %>%
+  filter(!is.na(meta_strata_2)) %>%
+  distinct(temp_strata_2, meta_strata_2)

+strata_mapping_1 <- setNames(mapping_1$meta_strata_1, mapping_1$temp_strata_1)
+strata_mapping_2 <- setNames(mapping_2$meta_strata_2, mapping_2$temp_strata_2)


nitpick: You can clean this up a tiny bit using set_names from rlang or purrr:

strata_mapping_2 <- assessment_data_pred %>% filter(!is.na(meta_strata_2)) %>% distinct(meta_strata_2, temp_strata_2) %>% purrr::set_names(.$meta_strata_2, .$temp_strata_2)

pipeline/02-assess.R

dfsnow · 2024-11-18T18:34:51Z

pipeline/02-assess.R

@@ -154,7 +183,8 @@ assessment_data_merged %>%
  select(
    meta_year, meta_pin, meta_class, meta_card_num, meta_lline_num,
    meta_modeling_group, ends_with("_num_sale"), pred_card_initial_fmv,
-    all_of(params$model$predictor$all), township_code
+    all_of(params$model$predictor$all),
+    meta_strata_is_imputed, township_code


suggestion: Let's change the prefix of this variable to flag_ since its purpose fits slightly better with that group of vars.

dfsnow · 2024-11-18T18:39:18Z

pipeline/02-assess.R

@@ -268,7 +296,8 @@ assessment_data_pin <- assessment_data_merged %>%
    meta_year, meta_pin, meta_pin10, meta_triad_code, meta_township_code,
    meta_nbhd_code, meta_tax_code, meta_class, meta_tieback_key_pin,
    meta_tieback_proration_rate, meta_cdu, meta_modeling_group,


Nope! This should just get automatically added to the equivalent tables in Athena once it's crawled.

Co-authored-by: Dan Snow <[email protected]>

Merge remote-tracking branch 'refs/remotes/origin/54-include-imputed-strata-in-assessment_data-assessment_card' into 54-include-imputed-strata-in-assessment_data-assessment_card # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Damonamajor · 2024-11-29T19:53:11Z

testing_file.R

@@ -0,0 +1,85 @@
+assessment_data_pred <- read_parquet(paths$input$assessment$local) %>%


This is a testing file demonstrating that the re-converted strata are the same as the original strata.

Merge remote-tracking branch 'refs/remotes/origin/54-include-imputed-strata-in-assessment_data-assessment_card' into 54-include-imputed-strata-in-assessment_data-assessment_card

dfsnow

@Damonamajor This looks fine for now. Let's review it as a part of our modeling work in the next couple weeks.

dfsnow · 2024-12-04T20:27:56Z

pipeline/02-assess.R

+  {
+    set_names(.$meta_strata_1, .$temp_strata_1)
+  }


nitpick: Why the additional curly braces here? You should be able to just:

Suggested change

{

set_names(.$meta_strata_1, .$temp_strata_1)

}

set_names(.$meta_strata_1, .$temp_strata_1)

…ta-in-assessment_data-assessment_card

ssaurbier · 2024-12-19T13:08:02Z

Hey all - this is a really simple issue. You need to use a specific random seed when modeling for consistent results. This is a basic best practice. Even if you somehow are getting deterministic results, it is an anti-pattern to not use random seeds.

https://towardsdatascience.com/how-to-use-random-seeds-effectively-54a4cd855a79

Random seeds absolutely must be used. There is no code problem here, you are just missing a random seed param.

Damonamajor added 11 commits November 13, 2024 16:30

Include testing file

a603001

include commenting for testing file

223aa02

remove code from pipeline

fb520c6

Add 3 enters

80fd36f

Update commenting

b692b22

update pipeline

6edaaf3

Add testing v2 file

3e1a54b

Testing file v2

3e4ab9b

Add concluding line

45accc7

include mapping

84593d4

Improve structure; add commenting

0837cd2

Damonamajor linked an issue Nov 18, 2024 that may be closed by this pull request

Include imputed strata in assessment_data / assessment_card #54

Closed

Remove testing files

8ae8d84

Damonamajor commented Nov 18, 2024

View reviewed changes

Damonamajor added 2 commits November 18, 2024 15:25

lintr

aeaa387

Add commenting

d0d8135

Damonamajor commented Nov 18, 2024

View reviewed changes

Damonamajor added 2 commits November 18, 2024 16:07

Add unname

0bf170c

Remove testing

75d8d4e

Damonamajor marked this pull request as ready for review November 18, 2024 16:16

Damonamajor requested review from dfsnow, wrridgeway and jeancochrane as code owners November 18, 2024 16:16

Damonamajor changed the title ~~Include imputed strata in assessment data assessment card~~ Include imputed strata in assessment card/pin Nov 18, 2024

dfsnow reviewed Nov 18, 2024

View reviewed changes

Damonamajor and others added 5 commits November 18, 2024 12:46

Update pipeline/02-assess.R

f22d226

Co-authored-by: Dan Snow <[email protected]>

Push testing file

92710bf

lintr

cfe990b

Add testing file

21ca2ca

Damonamajor added 2 commits November 18, 2024 20:34

Include True False

77d6cee

Include testing file

2d9628f

Damonamajor commented Nov 29, 2024

View reviewed changes

Damonamajor added 3 commits December 2, 2024 19:02

Change to flag

b08bdc7

Merge remote-tracking branch 'refs/remotes/origin/54-include-imputed-strata-in-assessment_data-assessment_card' into 54-include-imputed-strata-in-assessment_data-assessment_card

Make less than 80 characters

691fa3c

Final

1c4dfe6

Damonamajor requested a review from dfsnow December 3, 2024 02:27

dfsnow reviewed Dec 4, 2024

View reviewed changes

dfsnow mentioned this pull request Dec 7, 2024

Make model deterministic #61

Closed

dfsnow added 5 commits December 17, 2024 16:39

Drop testing file

ee5666f

Simplify strata deconversion code

85ff9e6

Fix ccao package function arg names

d97a9cf

Merge branch 'dfsnow/fix-ccao-arg-names' into 54-include-imputed-stra…

c2c4d70

…ta-in-assessment_data-assessment_card

Drop SV added later cols

efd3bf4

dfsnow changed the base branch from master to 2025-assessment-year December 17, 2024 20:38

dfsnow merged commit 4c55cc3 into 2025-assessment-year Dec 17, 2024
4 checks passed

dfsnow deleted the 54-include-imputed-strata-in-assessment_data-assessment_card branch December 17, 2024 20:39

Damonamajor mentioned this pull request Jan 7, 2025

56 add strata to desk review spreadsheets #80

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include imputed strata in assessment card/pin #55

Include imputed strata in assessment card/pin #55

Damonamajor commented Nov 18, 2024

Damonamajor Nov 18, 2024 •

edited

Loading

Damonamajor Nov 18, 2024 •

edited

Loading

dfsnow Nov 18, 2024

dfsnow left a comment

dfsnow Nov 18, 2024

Damonamajor Dec 2, 2024

dfsnow Nov 18, 2024

dfsnow Nov 18, 2024

dfsnow Nov 18, 2024

Damonamajor Nov 29, 2024

dfsnow left a comment

dfsnow Dec 4, 2024

Damonamajor Dec 4, 2024

ssaurbier commented Dec 19, 2024

		@@ -0,0 +1,85 @@
		assessment_data_pred <- read_parquet(paths$input$assessment$local) %>%

Include imputed strata in assessment card/pin #55

Include imputed strata in assessment card/pin #55

Conversation

Damonamajor commented Nov 18, 2024

Damonamajor Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Damonamajor Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfsnow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfsnow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssaurbier commented Dec 19, 2024

Damonamajor Nov 18, 2024 •

edited

Loading

Damonamajor Nov 18, 2024 •

edited

Loading