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

revise KPDB aggregations #666

Merged
merged 5 commits into from
Jul 10, 2024
Merged

revise KPDB aggregations #666

merged 5 commits into from
Jul 10, 2024

Conversation

damonmcc
Copy link
Member

@damonmcc damonmcc commented Mar 8, 2024

related to #665. this doesn't resolve it because this doesn't add senior_housing, nycha, and classb to aggregation outputs

see commit messages for all changes

successful build here

screenshots of new columns in aggregation outputs

Screenshot 2024-07-10 at 2 10 35 PM

Screenshot 2024-07-10 at 2 09 41 PM

Screenshot 2024-07-10 at 2 10 10 PM

Screenshot 2024-07-10 at 2 10 59 PM

@damonmcc damonmcc force-pushed the dm-kpdb-for-gis branch 2 times, most recently from dc7d043 to 3e48303 Compare March 10, 2024 17:37
@damonmcc damonmcc force-pushed the dm-kpdb-for-gis branch 3 times, most recently from fdf2e7a to 95355e3 Compare July 10, 2024 03:15
CASE
WHEN
has_future_units AND NOT has_project_phasing
NOT has_project_phasing AND completed_units = 0
Copy link
Contributor

@fvankrieken fvankrieken Jul 10, 2024

Choose a reason for hiding this comment

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

just clarifying this is intended as (NOT has_project_phasing) AND completed_units = 0 since that is the way postgres interprets it (vs NOT (has_project_phasing AND completed_units = 0))

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it just checking. Also - what's the reason to use logic on this unit value rather than its status?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes to ur first comment, think it's worth adding the parentheses?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope!

Copy link
Member Author

@damonmcc damonmcc Jul 10, 2024

Choose a reason for hiding this comment

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

logic on the phased unit values seems more accurate and easier to understand than using the status values

the status values do determine how units get allocated to the the different phases (within_5_years, from_5_to_10_years, after_10_years). I added the table summary_record_phasing to show record counts by source + status

Screenshot 2024-07-10 at 2 52 53 PM

@@ -23,7 +23,9 @@ future_units_with_cd AS (
units_gross,
Copy link
Contributor

Choose a reason for hiding this comment

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

these are real ripe to be built with dbt

Copy link
Contributor

Choose a reason for hiding this comment

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

(but that should wait till doing kpdb entirely - much prefer only the intermediate of testing with dbt, building entirely outside of it)

@damonmcc damonmcc merged commit 7d9969f into main Jul 10, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants