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.

Sign up for GitHub

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

Improve sales val dbt docs #607

Merged
merged 5 commits into from
Sep 24, 2024
Merged

Improve sales val dbt docs #607

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ccb2a5
Add improved docs and fix outlier_reason$n
wagnerlmichael Sep 24, 2024
db150ae
Add more info on rolling window
wagnerlmichael Sep 24, 2024
f53bb5a
Change language slightly
wagnerlmichael Sep 24, 2024
858aa2e
Add feedback
wagnerlmichael Sep 24, 2024
8d9c527
Add schema refs
wagnerlmichael Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions dbt/models/sale/schema.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ sources:
PTAX-203 form (regardless of statistical deviation)
- name: rolling_window
description: |
Rolling window period used to calculate statistics
for flagging this sale
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.
Comment on lines +18 to +20
Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a little more clear.

- name: run_id
description: '{{ doc("shared_column_sv_run_id") }}'
- name: sv_is_heuristic_outlier
Expand All @@ -26,11 +27,11 @@ sources:
- name: sv_is_ptax_outlier
description: '{{ doc("shared_column_sv_is_ptax_outlier") }}'
- name: sv_outlier_reason1
description: '{{ doc("shared_column_sv_outlier_reason") }}'
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") }}'
Copy link
Member Author

Choose a reason for hiding this comment

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

This was actually outdated

Copy link
Contributor

Choose a reason for hiding this comment

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

[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") }}'

- name: version
description: '{{ doc("shared_column_sv_version") }}'

Expand Down
44 changes: 43 additions & 1 deletion dbt/models/shared_columns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jeancochrane jeancochrane Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

{% enddocs %}

## is_common_area
Expand Down Expand Up @@ -1325,6 +1325,12 @@ Indicates an outlier sale not used in modeling or reporting.

This variable combines `sv_is_heuristic_outlier`
with `sv_is_ptax_outlier` (using OR logic).

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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


NOTE: Outlier flags only exist for sales _after_ 2014.
Copy link
Contributor

Choose a reason for hiding this comment

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

[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".

{% enddocs %}

Expand All @@ -1347,6 +1353,42 @@ See the [model-sales-val](https://github.com/ccao-data/model-sales-val)
repository for a list of possible flags.
{% enddocs %}

## 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

[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.


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 %}
Copy link
Contributor

Choose a reason for hiding this comment

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

[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 %}


## sv_run_id

{% docs shared_column_sv_run_id %}
Expand Down
Loading