Improve sales val dbt docs #607

wagnerlmichael · 2024-09-24T16:12:04Z

The original intent of this PR was to improve the docs; specifically explaining the null case for sv_is_outlier.

However we also had not updated the docs to reflect the changes made in this PR from this issue.

wagnerlmichael · 2024-09-24T16:19:58Z

dbt/models/sale/schema.yml

+            description: '{{ doc("shared_column_sv_outlier_reason1") }}'
          - name: sv_outlier_reason2
-            description: '{{ doc("shared_column_sv_outlier_reason") }}'
+            description: '{{ doc("shared_column_sv_outlier_reason2") }}'
          - name: sv_outlier_reason3
-            description: '{{ doc("shared_column_sv_outlier_reason") }}'
+            description: '{{ doc("shared_column_sv_outlier_reason3") }}'


This was actually outdated

[Nitpick, blocking] One small tweak here: Since the three outlier columns all share the same text for their descriptions, it actually fits our established style patterns for dbt docs to share a single description for them! So we can revert this change and just update the description for shared_column_sv_outlier_reason below.

- name: sv_outlier_reason1 description: '{{ doc("shared_column_sv_outlier_reason") }}' - name: sv_outlier_reason2 description: '{{ doc("shared_column_sv_outlier_reason") }}' - name: sv_outlier_reason2 description: '{{ doc("shared_column_sv_outlier_reason") }}'

wagnerlmichael · 2024-09-24T16:20:38Z

dbt/models/shared_columns.md

@@ -879,7 +879,7 @@ Parcel has an active homeowner exemption

 {% docs shared_column_is_ahsap %}
 Affordable Housing Special Assessment Program indicator. For more information on
-AHSAP, see: https://www.cookcountyassessor.com/affordable-housing
+AHSAP, see: <https://www.cookcountyassessor.com/affordable-housing>


Maybe a pre-commit thing is doing this. I can remove the arrows and --no-verify?

[Praise] I think it's fine to keep the arrows!

wagnerlmichael · 2024-09-24T16:23:48Z

dbt/models/sale/schema.yml

+              Rolling window period used to calculate grouping statistics
+              for flagging this sale. As of Sep 2024, ~1 year is the rolling window.
+              The month of the sale and the prior 11 months are used.


This might be a little more clear.

jeancochrane

Thanks for this! My only real concern is that we collapse the sv_outlier_reason descriptions into one shared description, given that they're all the same. Once that's done and you've resolved the rest of my comments to your liking, feel free to merge.

jeancochrane · 2024-09-24T16:45:59Z

dbt/models/shared_columns.md

@@ -879,7 +879,7 @@ Parcel has an active homeowner exemption

 {% docs shared_column_is_ahsap %}
 Affordable Housing Special Assessment Program indicator. For more information on
-AHSAP, see: https://www.cookcountyassessor.com/affordable-housing
+AHSAP, see: <https://www.cookcountyassessor.com/affordable-housing>


[Praise] I think it's fine to keep the arrows!

jeancochrane · 2024-09-24T16:48:04Z

dbt/models/shared_columns.md

+filters on type of sale or time frame of sale, is
+excluded completely from the sales-val model pipeline
+and therefor does recieve a boolean value.
+
 NOTE: Outlier flags only exist for sales _after_ 2014.


[Suggestion, non-blocking] Maybe we could make the connection here explicit:

Suggested change

NOTE: Outlier flags only exist for sales _after_ 2014.

NOTE: Outlier flags only exist for sales _after_ 2014.

Sales before 2014 will have a null value for this column.

Also, is the date range inclusive of 2014? If not, the above text should read "Sales including and before 2014".

jeancochrane · 2024-09-24T16:48:53Z

dbt/models/shared_columns.md

+A null value represents an observation that, due to
+filters on type of sale or time frame of sale, is
+excluded completely from the sales-val model pipeline
+and therefor does recieve a boolean value.


[Nitpick, non-blocking] Two small typos here:

Suggested change

and therefor does recieve a boolean value.

and therefore does receive a boolean value.

jeancochrane · 2024-09-24T16:51:17Z

dbt/models/sale/schema.yml

+            description: '{{ doc("shared_column_sv_outlier_reason1") }}'
          - name: sv_outlier_reason2
-            description: '{{ doc("shared_column_sv_outlier_reason") }}'
+            description: '{{ doc("shared_column_sv_outlier_reason2") }}'
          - name: sv_outlier_reason3
-            description: '{{ doc("shared_column_sv_outlier_reason") }}'
+            description: '{{ doc("shared_column_sv_outlier_reason3") }}'


[Nitpick, blocking] One small tweak here: Since the three outlier columns all share the same text for their descriptions, it actually fits our established style patterns for dbt docs to share a single description for them! So we can revert this change and just update the description for shared_column_sv_outlier_reason below.

- name: sv_outlier_reason1 description: '{{ doc("shared_column_sv_outlier_reason") }}' - name: sv_outlier_reason2 description: '{{ doc("shared_column_sv_outlier_reason") }}' - name: sv_outlier_reason2 description: '{{ doc("shared_column_sv_outlier_reason") }}'

jeancochrane · 2024-09-24T16:52:42Z

dbt/models/shared_columns.md

+## sv_outlier_reason1
+
+{% docs shared_column_sv_outlier_reason1 %}
+One of three possible reasons that a sale is
+flagged as on outlier. The priority for
+sv_outlier_reason$n column filling is
+ptax outlier > price outlier > characteric outlier.
+
+See the [model-sales-val](https://github.com/ccao-data/model-sales-val)
+repository for a list of possible flags.
+{% enddocs %}
+
+## sv_outlier_reason2
+
+{% docs shared_column_sv_outlier_reason2 %}
+One of three possible reasons that a sale is
+flagged as on outlier. The priority for
+sv_outlier_reason$n column filling is
+ptax outlier > price outlier > characteric outlier.
+
+See the [model-sales-val](https://github.com/ccao-data/model-sales-val)
+repository for a list of possible flags.
+{% enddocs %}
+
+## sv_outlier_reason3
+
+{% docs shared_column_sv_outlier_reason3 %}
+One of three possible reasons that a sale is
+flagged as on outlier. The priority for
+sv_outlier_reason$n column filling is
+ptax outlier > price outlier > characteric outlier.
+
+See the [model-sales-val](https://github.com/ccao-data/model-sales-val)
+repository for a list of possible flags.
+{% enddocs %}


[Nitpick, blocking] Per my comment above, we can collapse these into one description given that they share the exact same text:

Suggested change

## sv_outlier_reason1

{% docs shared_column_sv_outlier_reason1 %}

One of three possible reasons that a sale is

flagged as on outlier. The priority for

sv_outlier_reason$n column filling is

ptax outlier > price outlier > characteric outlier.

See the [model-sales-val](https://github.com/ccao-data/model-sales-val)

repository for a list of possible flags.

{% enddocs %}

## sv_outlier_reason2

{% docs shared_column_sv_outlier_reason2 %}

One of three possible reasons that a sale is

flagged as on outlier. The priority for

sv_outlier_reason$n column filling is

ptax outlier > price outlier > characteric outlier.

See the [model-sales-val](https://github.com/ccao-data/model-sales-val)

repository for a list of possible flags.

{% enddocs %}

## sv_outlier_reason3

{% docs shared_column_sv_outlier_reason3 %}

One of three possible reasons that a sale is

flagged as on outlier. The priority for

sv_outlier_reason$n column filling is

ptax outlier > price outlier > characteric outlier.

See the [model-sales-val](https://github.com/ccao-data/model-sales-val)

repository for a list of possible flags.

{% enddocs %}

## sv_outlier_reason

{% docs shared_column_sv_outlier_reason %}

One of three possible reasons that a sale is

flagged as on outlier. The priority for

sv_outlier_reason$n column filling is

ptax outlier > price outlier > characteristic outlier.

See the [model-sales-val](https://github.com/ccao-data/model-sales-val)

repository for a list of possible flags.

{% enddocs %}

jeancochrane · 2024-09-24T16:53:19Z

dbt/models/shared_columns.md

+One of three possible reasons that a sale is
+flagged as on outlier. The priority for
+sv_outlier_reason$n column filling is
+ptax outlier > price outlier > characteric outlier.


[Nitpick, non-blocking] Small typo here, characteric should be characteristic. No suggestion for this change, since I include this fix in my bigger-picture suggestion below.

wagnerlmichael added 3 commits September 24, 2024 16:08

Add improved docs and fix outlier_reason$n

0ccb2a5

Add more info on rolling window

db150ae

Change language slightly

f53bb5a

wagnerlmichael commented Sep 24, 2024

View reviewed changes

wagnerlmichael marked this pull request as ready for review September 24, 2024 16:24

wagnerlmichael requested a review from a team as a code owner September 24, 2024 16:24

jeancochrane approved these changes Sep 24, 2024

View reviewed changes

wagnerlmichael added 2 commits September 24, 2024 18:19

Add feedback

858aa2e

Add schema refs

8d9c527

wagnerlmichael merged commit 01802aa into master Sep 24, 2024
7 checks passed

wagnerlmichael deleted the improve-sales-val-docs branch September 24, 2024 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve sales val dbt docs #607

Improve sales val dbt docs #607

wagnerlmichael commented Sep 24, 2024 •

edited

Loading

wagnerlmichael Sep 24, 2024

jeancochrane Sep 24, 2024

wagnerlmichael Sep 24, 2024

jeancochrane Sep 24, 2024 •

edited

Loading

wagnerlmichael Sep 24, 2024

jeancochrane left a comment

jeancochrane Sep 24, 2024 •

edited

Loading

jeancochrane Sep 24, 2024

jeancochrane Sep 24, 2024

jeancochrane Sep 24, 2024

jeancochrane Sep 24, 2024

jeancochrane Sep 24, 2024

	NOTE: Outlier flags only exist for sales _after_ 2014.
	NOTE: Outlier flags only exist for sales _after_ 2014.
	Sales before 2014 will have a null value for this column.

	and therefor does recieve a boolean value.
	and therefore does receive a boolean value.

Improve sales val dbt docs #607

Improve sales val dbt docs #607

Conversation

wagnerlmichael commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane left a comment

Choose a reason for hiding this comment

jeancochrane Sep 24, 2024 • edited Loading

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

wagnerlmichael commented Sep 24, 2024 •

edited

Loading

jeancochrane Sep 24, 2024 •

edited

Loading

jeancochrane Sep 24, 2024 •

edited

Loading