Add pin class to pinval.assessment_card

[wagnerlmichael]

Bring in the pin class to pinval.assessment_card. Supports ccao-data/homeval#155

(PR #1000 btw)

[jeancochrane]

Two small tweaks for you, plus one task for me to investigate before we merge!

Also, congrats on PR 1,000 🎉

[jeancochrane]

[Nitpick, blocking] Hmm, there shouldn't be this many ineligible reports. Let me take a look at this before we merge.

[jeancochrane]

OK, I think I have a proposal of how we can resolve these errors for good. This query demonstrates that all the PINs that are failing this check are PINs that have regression-class cards, but non-regression-class PINs:

select
    card.meta_pin,
    card.meta_card_num,
    card.pin_class,
    card.char_class,
    card.is_report_eligible,
    card.reason_report_ineligible,
    class_dict.regression_class,
    class_dict.modeling_group
from z_ci_add_pin_class_to_custom_homeval_message_pinval.assessment_card as card
left join ccao.class_dict as class_dict
    on card.pin_class = class_dict.class_code
where card.is_report_eligible
    and card.reason_report_ineligible is not null

In the past, we've chosen not to consider the card class or PIN class when computing is_report_eligible because we believed that any mismatches could be a useful flag that the model is valuing PINs that have non-regression classes. However, after thinking about it more, I think this is not the right place to implement that flag, because HomeVal is ultimately a totally separate process from modeling and we don't want modeling errors to block HomeVal changes.

I think the easiest fix, then, would be to consider PIN class when computing is_report_eligible. If a PIN is not a regression class, we should not show a report for it, even if the model valued it; I think this makes sense because the report will be misleading in those cases.

Here's the block we'll need to change to consider PIN class when computing is_report_eligible:

data-architecture/dbt/models/pinval/pinval.assessment_card.sql

Lines 247 to 285 in cc4758e

	-- Three possible reasons we would decline to build a PINVAL report for a
	-- PIN:
	--
	-- 1. No representation of the PIN in assessment_card because it is
	-- not a regression class and so was excluded from the assessment set
	-- 2. PIN has a row in `model.assessment_card`, but no card number,
	-- indicating a rare data error
	-- 3. PIN tri is not up for reassessment
	-- - These PINs are still included in the assessment set, they just
	-- do not receive final model values
	--
	-- It's important that we get this right because PINVAL reports will
	-- use this indicator to determine whether to render a report. As such,
	-- the conditions in this column are a bit more lax than the conditions
	-- in the `reason_report_ineligible` column, because we want to catch cases
	-- where PINs are unexpectedly eligible for reports.
	--
	-- Also note that the 'unknown' conditional case for
	-- the `reason_report_ineligible` column mirrors this logic in its column
	-- definition, so if you change this logic, you should also change that
	-- conditional case
	(
	ac.meta_pin IS NOT NULL
	AND ac.meta_card_num IS NOT NULL
	AND LOWER(uni.triad_name) = LOWER(uni.assessment_triad)
	-- Fix for one 2024 PIN that switched from regression class to
	-- non-regression class between the date when we ran the final model
	-- and the date we ran final comps. Without the filter below, this view
	-- will consider the PIN to be eligible for a report, because it had
	-- a regression class at the time of final modeling; however, the PIN
	-- doesn't have any comps, because it had a non-regression class at the
	-- time of comps.
	--
	-- An alternative approach might be to use the presence of comps to
	-- determine report eligibilty, but eligible reports without comps
	-- can be a useful signal of something going wrong with our eligibility
	-- criteria, so instead we hardcode this exception.
	AND NOT (ac.meta_pin = '10361150280000' AND ac.assessment_year = '2024')
	) AS is_report_eligible,

We don't need to copy the changes to reason_report_ineligible because they are already implemented there:

data-architecture/dbt/models/pinval/pinval.assessment_card.sql

Lines 286 to 307 in cc4758e

	CASE
	-- In some rare cases the parcel class can be different from
	-- the card class, in which case these class explanations are not
	-- guaranteed to be the true reason that a report is missing. But
	-- in those cases, a non-regression class for the PIN should still be
	-- a valid reason for a report to be unavailable, so we report it
	-- as a best guess at true reason why the report is missing
	WHEN uni.class IN ('299') THEN 'condo'
	WHEN
	pin_cd.class_code IS NULL -- Class is not in our class dict
	OR NOT pin_cd.regression_class
	OR (pin_cd.modeling_group NOT IN ('SF', 'MF', 'BB'))
	THEN 'non_regression_class'
	WHEN LOWER(uni.triad_name) != LOWER(uni.assessment_triad) THEN 'non_tri'
	WHEN ac.meta_card_num IS NULL THEN 'missing_card'
	WHEN
	ac.meta_pin IS NOT NULL
	AND ac.meta_card_num IS NOT NULL
	AND LOWER(uni.triad_name) = LOWER(uni.assessment_triad)
	THEN NULL
	ELSE 'unknown'
	END AS reason_report_ineligible,

[jeancochrane]

I'm glad we can resolve the pinval_assessment_card_reason_report_ineligible_is_null_when_is_report_eligible errors while also improving the query. Nice work!

[jeancochrane]

[Suggestion, optional] Now that we're using pin_class as the basis of the non-regression class check in is_report_eligible, I think we no longer need a hardcoded exception for this PIN, because it now has regression_class = FALSE:

select
    card.meta_pin,
    card.meta_card_num,
    card.pin_class,
    card.char_class,
    class_dict.regression_class,
    class_dict.modeling_group
from z_ci_add_pin_class_to_custom_homeval_message_pinval.assessment_card as card
left join ccao.class_dict as class_dict
    on card.pin_class = class_dict.class_code
where card.meta_pin = '10361150280000' and card.assessment_year = '2024'

meta_pin	meta_card_num	pin_class	char_class	regression_class	modeling_group
10361150280000	1	100	204	false

[wagnerlmichael]

Thanks for confirming this

[jeancochrane]

Ready to roll!