Skip to content

Warn instead of silently mislabelling a dataset's fiscal year - #1846

Open
vahid-ahmadi wants to merge 1 commit into
mainfrom
fix/dataset-fiscal-year-default
Open

Warn instead of silently mislabelling a dataset's fiscal year#1846
vahid-ahmadi wants to merge 1 commit into
mainfrom
fix/dataset-fiscal-year-default

Conversation

@vahid-ahmadi

Copy link
Copy Markdown
Collaborator

Closes #1845.

UKSingleYearDataset.__init__ declared fiscal_year: int = 2025 but used it on only one of its two branches, so both halves failed quietly.

Ignored alongside a file path. With file_path, the period comes from the file and the argument was discarded without comment:

frs_2024_25.h5                     -> time_period = 2024
  same file with fiscal_year=2030  -> time_period = 2024   (silently ignored)

A hardcoded default that drifts. Building from DataFrames without it labelled the data 2025 — already a year ahead of the newest release, since FRS 2024/25 is label 2024 on our convention, and wrong differently every year:

UKSingleYearDataset(person=..., benunit=..., household=...)  -> 2025

This came up with an external collaborator who was labelling 2024/25 data as 2025 and reasonably assumed the argument did something.

This PR

fiscal_year now defaults to None, and both silent paths warn:

  • passing it with file_path raises a UserWarning saying it cannot take effect, and points at uprate_dataset for changing the period;
  • omitting it on the DataFrame branch raises a DeprecationWarning naming the label it fell back to and restating the convention.

Behaviour is otherwise unchanged: the fallback still yields 2025, kept as DEFAULT_FISCAL_YEAR. Nothing we ship is affected — create_frs passes fiscal_year at all three construction sites.

Why deprecate rather than require

Making fiscal_year mandatory is the better end state, but it breaks any caller relying on the default, so I have not done it here. If you would rather take the break now, say and I will switch the DeprecationWarning to a ValueError — it is a two-line change and the tests already cover both paths.

Tests

test_dataset_fiscal_year.py: the argument sets the period; omitting it warns and falls back; passing it with a file path warns and the file's period still wins; loading normally warns about nothing.

Full suite: 1,140 yaml policy tests and 181 pytest (44 skipped), ruff clean.

`UKSingleYearDataset` took `fiscal_year=2025` as a default but used it only
when building from DataFrames: with a file path the period is read from the
file and the argument was ignored without comment. Both halves failed
quietly. Passing a conflicting year alongside a file had no effect, and
omitting it when assembling frames by hand labelled the data 2025 — already
a year ahead of the newest release, and wrong differently every year.

The default now warns on use and the conflicting-argument case warns too,
so neither can mislabel a dataset unnoticed. Removing the default outright
would break callers, so it is deprecated rather than dropped.

Closes #1845

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@juaristi22

Copy link
Copy Markdown
Collaborator

Program Review

Source Documents

Critical (Must Fix)

  1. [C1] Both added warnings are suppressed in ordinary use. The new calls in dataset_schema.py:59 and dataset_schema.py:78 cannot reach callers under the package's normal import path. Importing policyengine_uk loads LHA modules such as LHA_category.py:6, which installs a process-wide warnings.filterwarnings("ignore"); the first active filter becomes ignore for every Warning subclass. A fresh-interpreter reproduction recorded default-path=0 file-path=0, so neither the DeprecationWarning nor the UserWarning was shown. The new pytest.warns(...) contexts force matching warnings visible and therefore pass while masking the integration failure. Scope/remove those global suppressors (and use an end-user-visible category such as FutureWarning for the fallback deprecation), then add a fresh-process regression under ordinary filters. Python documents that DeprecationWarning is otherwise ignored by default outside __main__, while FutureWarning targets application end users. (Python warning categories)

  2. [C2] The changelog file is invisible to Towncrier. changelog.d/dataset-fiscal-year-default.md:1 omits a configured fragment type. Repository guidance requires changelog.d/{slug}.{added|fixed|changed|...}.md; running towncrier build --draft at the reviewed head produced No significant changes. Rename this fix fragment to changelog.d/dataset-fiscal-year-default.fixed.md (or another configured type) so release automation consumes it.

Should Address

None.

Suggestions

None.

PDF Audit Summary

Category Count
Confirmed correct N/A
Mismatches (code-path confirmed + visually verified) N/A
Mismatches rejected (code-path cleared) N/A
Unmodeled items N/A
Pre-existing issues N/A

Validation Summary

Check Result
Regulatory Accuracy Not applicable
Reference Quality Not applicable
Code Patterns 2 critical issues
Formatting Ruff check passed; Ruff format check passed
Test Coverage Focused tests pass, but warning visibility is not tested under ordinary filters
Focused Tests 4 passed
PDF Value Audit Not applicable
CI Status All 6 checks passed, including the 14m11s broad Test check

Review Severity: REQUEST_CHANGES

Next Steps

Resolve C1 and C2, then rerun the fresh-process warning regression, the focused tests, Ruff, Towncrier draft, and the repository test suite. To auto-fix issues: run the fix-pr workflow for this PR.

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.

UKSingleYearDataset silently ignores fiscal_year with file_path, and defaults it to a hardcoded 2025

2 participants