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

feat: ofsted data points optional for gias submission #1794

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

J05h-L
Copy link
Collaborator

@J05h-L J05h-L commented Jan 22, 2025

Context

AB#241881 - AB#246016

Change proposed in this pull request

New gias input schema created. Following the existing pattern for input schemas default for the existing schema for historic processing and a new 2024 with the following columns omitted.

  • OfstedRating (name)
  • OfstedLastInsp

Changes to prepare_schools_data in pre_processing\ancillary\schools.py to accommodate the above.

The omitted columns are required to write to the db and OfstedRating (name) is a factor for rag calculations.

Therefore if not present in the submission these columns are created with default values.

  • OfstedRating (name) as an empty string. Later in rag.py this will resolve to "other" and not be considered for rag rating calculations. ie != "outstanding"
  • OfstedLastInsp as None (NaT for pd.to_datetime, NULL for the db)
    This aligns with current behaviour when these values are missing for a row.

If present in the submission the current behaviour is preserved.

Tests added to confirm the above.

Guidance to review

When running locally no changes to 2021 - 2023 default runs should be observed.

For 2024 all_schools.parquet OfstedRating (name) and OfstedLastInsp should only contain the default values.

Also for a 2024 run the latest gias all establishment data > establishment fields can be processed without error.

Checklist (add/remove as appropriate)

  • Work items have been linked (use AB#)
  • Your code builds clean without any errors or warnings
  • You have run all unit/integration tests and they pass
  • Your branch has been rebased onto main
  • You have tested by running locally

@J05h-L J05h-L force-pushed the feature/241881/ofsted-gias-schema branch from 7be3c3c to 25c0ffc Compare January 22, 2025 13:40
@J05h-L J05h-L marked this pull request as ready for review January 22, 2025 13:43
@J05h-L J05h-L force-pushed the feature/241881/ofsted-gias-schema branch 2 times, most recently from 25c0ffc to 4617ff2 Compare January 23, 2025 09:37
jrabbott
jrabbott previously approved these changes Jan 23, 2025
Copy link
Collaborator

@jrabbott jrabbott left a comment

Choose a reason for hiding this comment

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

LGTM - would appreciate @PsypherPunk eyes on the mappings

@jrabbott jrabbott requested a review from PsypherPunk January 23, 2025 10:22
Copy link
Collaborator

@PsypherPunk PsypherPunk left a comment

Choose a reason for hiding this comment

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

Functionally, with one stray print(), this LGTM; a couple of minor quibbles.

…data processing use default values.

    - new 2024 gias schema without `OfstedRating (name)` and `OfstedLastInsp` columns.
    - when ofsted related columns are missing they are created and set to default values for downstream processing and computations.
    - when ofsted related columns are present (for historic submissions) the current behaviour is preserved.
@J05h-L J05h-L force-pushed the feature/241881/ofsted-gias-schema branch from 4617ff2 to ea4805e Compare January 23, 2025 15:13
@J05h-L J05h-L requested a review from PsypherPunk January 23, 2025 15:17
@J05h-L J05h-L merged commit 03b9d89 into main Jan 23, 2025
13 checks passed
@J05h-L J05h-L deleted the feature/241881/ofsted-gias-schema branch January 23, 2025 16:00
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