Skip to content

Add cumulative enrollment and 6-year graduation metrics#3356

Open
anthonygwalters wants to merge 11 commits intomainfrom
claude/review-kfwd-persistence-model-dYUQA
Open

Add cumulative enrollment and 6-year graduation metrics#3356
anthonygwalters wants to merge 11 commits intomainfrom
claude/review-kfwd-persistence-model-dYUQA

Conversation

@anthonygwalters
Copy link
Copy Markdown
Member

Pull Request

Summary & Motivation

When merged, this pull request will add new metrics to the persistence model to track cumulative semesters enrolled and 6-year graduation outcomes by degree type. These metrics enable better analysis of student persistence patterns and graduation rates in the KFWD persistence reporting.

Changes

  • Added enrollment_by_semester CTE to aggregate enrollment status by semester
  • Added cumulative_enrollment CTE to calculate cumulative semesters enrolled using a window function
  • Added three new graduation metrics:
    • is_6yr_graduate_any: Whether student graduated (any degree type)
    • is_6yr_graduate_ba: Whether student graduated with a Bachelor's degree
    • is_6yr_graduate_aa: Whether student graduated with an Associate's degree
  • Added cumulative_semesters_enrolled field to the persistence model
  • Exposed new fields in the Tableau persistence report

Self-review

General

  • If this is a same-day request, please flag that in the #data-team Slack
  • Update due date and assignee on the TEAMster Asana Project
  • Run Format on all modified files

dbt

  • Include a corresponding [model name].yml properties file for all models
  • Include (or update) an exposure for all models that will be consumed by a dashboard

SQL

  • All distinct usage must be accompanied by a comment explaining its necessity
  • Do not use order by for select statements

Test Plan

Existing dbt tests should pass. Verify that:

  • The new CTEs correctly aggregate enrollment data by semester
  • Cumulative enrollment calculations are accurate across the student population
  • Graduation metrics correctly identify graduates by degree type in the Tableau report

https://claude.ai/code/session_016VscmMbi8kie1s8VDwsUt7

…gs to persistence model

Adds four new columns to int_kippadb__persistence (exposed in rpt_tableau__kfwd_persistence):

- cumulative_semesters_enrolled: running count of semesters a student was
  enrolled as of each semester (inclusive). Collapses multiple concurrent
  enrollments per semester via max(is_persisting_int) before summing.

- is_6yr_graduate_any / _ba / _aa: static boolean per student indicating
  whether they graduated (any path, BA only, or AA only) within the 6-year
  scaffold window. Uses logical_or() over the full student partition so the
  value is consistent across all semester rows.

https://claude.ai/code/session_016VscmMbi8kie1s8VDwsUt7
@anthonygwalters anthonygwalters requested a review from a team as a code owner March 2, 2026 21:02
@anthonygwalters anthonygwalters requested a review from a team March 2, 2026 21:02
Anthony Walters and others added 3 commits March 2, 2026 21:03
…laude/review-kfwd-persistence-model-dYUQA
…gs to persistence model

Adds four new columns to int_kippadb__persistence (exposed in rpt_tableau__kfwd_persistence):

- cumulative_semesters_enrolled: running count of semesters a student was
  enrolled as of each semester (inclusive). Collapses multiple concurrent
  enrollments per semester via max(is_persisting_int) before summing.

- is_6yr_graduate_any / _ba / _aa: static boolean per student indicating
  whether they graduated (any path, BA only, or AA only) within the 6-year
  scaffold window. Uses logical_or() over the full student partition so the
  value is consistent across all semester rows.

https://claude.ai/code/session_016VscmMbi8kie1s8VDwsUt7
Add cumulative_semesters_enrolled, is_6yr_graduate_any, is_6yr_graduate_ba,
and is_6yr_graduate_aa to the schema files for int_kippadb__persistence and
rpt_tableau__kfwd_persistence.

https://claude.ai/code/session_016VscmMbi8kie1s8VDwsUt7
@anthonygwalters anthonygwalters force-pushed the claude/review-kfwd-persistence-model-dYUQA branch from d182953 to 47fa9f4 Compare March 2, 2026 21:08
@cbini
Copy link
Copy Markdown
Member

cbini commented Mar 2, 2026

@anthonygwalters I'm adding some uniqueness testing, should every student only have one row per year and semester? this pulls in all enrollments during that window so some students have multiple

@anthonygwalters
Copy link
Copy Markdown
Member Author

@anthonygwalters I'm adding some uniqueness testing, should every student only have one row per year and semester? this pulls in all enrollments during that window so some students have multiple

that's unavoidable unfortunately, hence the rn

@cbini
Copy link
Copy Markdown
Member

cbini commented Mar 2, 2026

rn_enrollment_year? should we filter that in the main select statement? we could also dedupe stg_kippadb__enrollment in a CTE

@cbini cbini force-pushed the claude/review-kfwd-persistence-model-dYUQA branch from 70764be to efbf873 Compare March 4, 2026 19:25
@cbini cbini requested review from a team as code owners March 4, 2026 19:25
@cbini
Copy link
Copy Markdown
Member

cbini commented Mar 9, 2026

@claude review this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 9, 2026

Claude finished @cbini's task in 1m 38s —— View job


PR Review

  • Gather context and read changed files
  • Review SQL logic and CTEs
  • Review YAML property files
  • Check for issues raised in existing comments
  • Post final review

Overall the new CTEs are cleanly structured. One critical issue must be resolved before merging, plus a few smaller observations.

Critical: Uniqueness test will fail — unresolved multi-enrollment

The int_kippadb__persistence.yml now declares a uniqueness test on (student_number, persistence_year, semester):

- dbt_utils.unique_combination_of_columns:
    combination_of_columns:
      - student_number
      - persistence_year
      - semester

But the final SELECT reads directly from persistence_union without filtering on rn_enrollment_year. A student with two concurrent enrollments in the same semester produces two rows with identical (student_number, persistence_year, semester) — this test will fail.

This is exactly what the open comment thread is about. rn_enrollment_year is computed and exposed but never used as a filter. The fix cbini suggested is the right one: add where p.rn_enrollment_year = 1 to the final select. Fix this →

Note: Adding where rn_enrollment_year = 1 means only the most recent enrollment per student/year is used to compute the is_6yr_graduate_* window functions. For students who graduated from a non-primary enrollment (e.g. a transferred school), verify that the order by e.start_date desc ranking correctly surfaces the "right" row with persistence_status = 'Graduated' — otherwise graduation status could be silently dropped.


Medium: semester asc ordering is implicit

int_kippadb__persistence.sql lines 335–337:

order by persistence_year asc, semester asc

'Fall' < 'Spring' alphabetically, which happens to be correct chronologically. This is fragile — any future value like 'Summer' would silently break the ordering. A more explicit sort is safer:

order by persistence_year asc, case when semester = 'Fall' then 1 else 2 end asc

Fix this →


Low: enrollment_by_semester aggregation approach is sound

Using max(coalesce(is_persisting_int, 0)) to collapse multiple enrollment rows per semester is the right call — it correctly sets was_enrolled = 1 if any enrollment was active, and treats future semesters (where is_persisting_int is null) as 0. No issue here, just confirming the logic.


Pre-existing gap: rpt_tableau__kfwd_persistence.yml is missing contract and uniqueness test

Per kipptaf/CLAUDE.md, all rpt_ models must have contract: enforced: true and a uniqueness test. Neither is present in rpt_tableau__kfwd_persistence.yml. This predates the PR, but since the file is being modified it's a good opportunity to add them.


Summary

Block on merge Missing rn_enrollment_year = 1 filter — uniqueness test will fail
Recommend fixing Implicit semester asc sort ordering
Opportunistic Add contract + uniqueness test to rpt_tableau__kfwd_persistence.yml

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.

3 participants