Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

56 add strata to desk review spreadsheets #80

Merged

Conversation

Damonamajor
Copy link
Contributor

@Damonamajor Damonamajor commented Jan 7, 2025

This closes #56 which is the result of creating a flag for imputed strata values and recoding values through this #55.

Reviewing the spreadsheet, it seems as though columns sale_recent_1_sv_added_later through model_org_fmv_pct_chg are NA. This occurs when the spreadsheet is run from the 2025-assessment branch.

@Damonamajor Damonamajor linked an issue Jan 7, 2025 that may be closed by this pull request
Copy link
Member

@dfsnow dfsnow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Damonamajor Per my note on this in-person, you'll also need to modify the template file (misc/desk_review_template.xlsx) and the lines below this to insert the new columns into the Excel workbook.

Damonamajor and others added 15 commits January 9, 2025 23:01
* Add gaa to eval run_id variable

* Hide source code for conditionally included Quarto chunks

* Fix code echoed in Quarto doc

* Revert default run_id
* Fix missing geography_id for triads

* Remove geography_id in mutate()
* Add test

* Update reports/performance/_outliers.qmd

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

* lintr

---------

Co-authored-by: Dan Snow <[email protected]>
* Replace arrows with equal signs

* Revert unload change
* Update params to match residential model

* Hide echoed code in model reports

* Plot variance wrt time

* Update training data

* Remove multi-card filter

* Update DVC lockfile
Merge branch '2025-assessment-year' of github.com:ccao-data/model-condo-avm into 56-add-strata-to-desk-review-spreadsheets

# Conflicts:
#	renv.lock
#	reports/_setup.R
#	reports/performance/_model.qmd
Merge branch '2025-assessment-year' of github.com:ccao-data/model-condo-avm into 56-add-strata-to-desk-review-spreadsheets
# 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.
char_unit_sf, flag_nonlivable_space, flag_pin10_5yr_num_sale,
flag_common_area, flag_proration_sum_not_1, flag_pin_is_multiland,
flag_land_gte_95_percentile,
char_unit_sf, meta_strata_1, meta_strata_2, flag_nonlivable_space,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flag_common_area was removed

@Damonamajor
Copy link
Contributor Author

@jeancochrane - same NA values occur when running from the 2025-assessment branch. Do we want a new issue to debug that?

Copy link
Contributor

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, thanks! A couple tiny formatting nits with the new template -- no need to implement these now, we can clean up all the tiny stuff when we do a final pass for export:

  • "Sale Num. Parcels 2" header cell needs a right border in order to close out "Sales Information" section
  • Move right border from "Condo Unit S.F." header cell to "Strata 2" in order to close out "Characteristics" section
  • Move right border from "YoY Change <= -5%" header cell to "Strata is Imputed" in order to close out "Flags" section
  • "Strata is Imputed" header cell needs a hoverable note to explain what it is, similar to the other header cells in the "Flags" section
  • "% Change" header cell needs a right border in order to close out "Compared to..." section

@@ -80,7 +80,6 @@ land <- dbGetQuery(




Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question, non-blocking] Is this intentional? Elsewhere in the PR it seems like 4 spaces is the standard between major sections, but I'm not totally sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -200,7 +200,6 @@ assessment_pin10_prepped <- assessment_pin_prepped %>%




Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question, non-blocking] Same question here as above.

@Damonamajor
Copy link
Contributor Author

@jeancochrane
I updated the tiny fixes and removed the columns from #96

Copy link
Contributor

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks!

@Damonamajor Damonamajor merged commit 666c375 into 2025-assessment-year Feb 3, 2025
4 checks passed
@Damonamajor Damonamajor deleted the 56-add-strata-to-desk-review-spreadsheets branch February 3, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add strata to Desk Review spreadsheets
4 participants