Repository navigation
remove tretspef grouped - #700
Open
tomjemmett wants to merge 5 commits into
Open
tomjemmett wants to merge 5 commits into
tomjemmett wants to merge 5 commits into
Conversation
* remove the trestpef_grouped column, instead use tretspef * sex+tretspef_grouped aggregation replaced with sex+tretspef * tretspef aggregation removed as redundant (sex+tretspef will give same results when you aggregate without sex column)
✅ A new build is available.You can use the following to use pull the image into your local environment: docker pull ghcr.io/the-strategy-unit/nhp_model:pr-700 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 1216 1214 -2
Branches 61 61
=========================================
- Hits 1216 1214 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The change alters model output semantics and relies on large regenerated integration/e2e snapshots and a coordinated cross-repo params-format change that cannot be verified without the (unavailable) model data, warranting human review.
No open issues 🎉
What changed in this PR
This PR implements issue #699 by removing the tretspef_grouped column from the NHP demand model, so the model keys all treatment-specialty logic on the raw tretspef column instead. This is coordinated with upstream removal of the column in the nhp_data/nhp_inputs pipelines. The core source change is small and mechanical; most of the diff is a large expansion of the params JSON schema (raw tretspef codes replacing the Other (Medical)/Other (Surgical) groupings) and regenerated integration/e2e snapshot fixtures.
Changes:
- Replace
tretspef_groupedwithtretspefin groupings, aggregations, and resampling index names; collapse thesex+tretspef_grouped+ standalonetretspefaggregations into a singlesex+tretspefaggregation. - Restructure
aaeadjustment params to nest values under"Other"(schema + sample), and drop the now-redundantbaseline_adjustment.aaeruntime patch inmodel_iteration._patch_run_params; expand thetretspef_groups_*schema defs to the full raw-specialty code list. - Update unit tests and regenerate integration/e2e golden fixtures to reflect the new column/aggregation behavior.
| File | Description |
|---|---|
| src/nhp/model/activity_resampling.py | Rename factor MultiIndex level tretspef_grouped → tretspef in expat/repat/baseline/waiting-list adjustments. |
| src/nhp/model/inpatients.py | Drop tretspef_grouped from groupings/sdec assignment and merge the two tretspef aggregations into sex+tretspef. |
| src/nhp/model/outpatients.py | Same removal of tretspef_grouped from grouping columns and aggregations. |
| src/nhp/model/model_iteration.py | Remove the baseline_adjustment.aae nesting patch, now provided directly by params. |
| src/nhp/model/params/params-schema.json | Replace grouped-specialty schema with full raw tretspef code lists; nest baseline_adjustment.aae under Other. |
| src/nhp/model/params/params-sample.json | Update sample params to raw Other keys and nested aae structure. |
| tests/unit/nhp/model/test_inpatients.py | Remove tretspef_grouped fixtures/expectations and update aggregation assertions. |
| tests/unit/nhp/model/test_outpatients.py | Same unit-test updates for outpatients aggregations. |
| tests/unit/nhp/model/test_activity_resampling.py | Rename test data column tretspef_grouped → tretspef. |
| tests/unit/nhp/model/test_model_iteration.py | Drop baseline_adjustment from _patch_run_params input/expected. |
| tests/integration/.../test_model_returns_expected_aggregations.yml | Update expected aggregation keys to sex+tretspef. |
| tests/integration/.../test_model_default_results_ip_.csv | Regenerated IP default-results snapshot. |
| tests/integration/.../test_model_default_results_op_.csv | Regenerated OP default-results snapshot. |
| tests/integration/.../test_model_default_results_aae_.csv | Regenerated A&E default-results snapshot. |
| tests/integration/.../test_model_step_counts_op_.csv | Regenerated OP step-counts snapshot. |
| tests/integration/.../test_model_step_counts_aae_.csv | Regenerated A&E step-counts snapshot. |
| tests/e2e/.../test_model_results_returns_expected_keys.yml | Update expected result keys to sex+tretspef. |
| tests/e2e/.../test_all_model_runs_default_.csv | Regenerated full-run e2e snapshot. |
Review effort: Balanced
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #699
Stack created with GitHub Stacks CLI • Give Feedback 💬