[FibrinLab]

Fixes issue: mid-parental-height accepts impossible values #53

• Add MAXIMUM_PARENTAL_HEIGHT_CM constant (260 cm) to validation_constants.py
• Add comprehensive input validation to mid_parental_height() function
• Add input validation to mid_parental_height_z() function
• Add 7 new test functions covering all validation scenarios

[mbarton]

Thanks for the contribution @FibrinLab! It looks fine from a code point of view however we will need to run this past the statisticians who advise us before merging. Hopefully won't take too long :)

[eatyourpeas]

Good job @FibrinLab thank you. The reason there is no error handling in the python package is because we put it in the server - see here: https://github.com/rcpch/digital-growth-charts-server/blob/live/routers/utilities.py
Actually though you make a really good point that we should have this in the package too for peope who consume it without the API.

I can see you have used the Guiness Book of Records values to set min and max. We did deprecate this in favour of using SDS cut offs - might you be happy to review the approach taken above in the server and then replicate here? That way I can remove the code in the server and just trap the errors raised in the package and return those to the user?

[FibrinLab]

Hello @eatyourpeas @mbarton
As per your recommendation, I have switched to SDS cutoffs.

[eatyourpeas]

Thank you @FibrinLab I have spun it up and it looks good, all your tests are passing. I will need a little time to integrate it with the server as there will need to be code there that will trap the errors from here. So if this sits for a little while before merging that is the reason.

[FibrinLab]

Thank you @FibrinLab I have spun it up and it looks good, all your tests are passing. I will need a little time to integrate it with the server as there will need to be code there that will trap the errors from here. So if this sits for a little while before merging that is the reason.

Would love to work on another issue, which do you recommend @mbarton ?

[eatyourpeas]

you could potentially finish this off by reviewing the midparental height error handling in the server and integrating it with what you have here? The response from the api would need to be the same, so it would just be a question of removing the code there that validates the MPH by SDS and passes the errors you raise here through to the user. Would you be happy to have a go there? It is a FastAPI project, hopefully self-explanatory but do post questions if more guidance needed.

I have created an issue in the server for you to attach an pull requests to.

Thanks again for your help with this project.

[FibrinLab]

you could potentially finish this off by reviewing the midparental height error handling in the server and integrating it with what you have here? The response from the api would need to be the same, so it would just be a question of removing the code there that validates the MPH by SDS and passes the errors you raise here through to the user. Would you be happy to have a go there? It is a FastAPI project, hopefully self-explanatory but do post questions if more guidance needed.

I have created an issue in the server for you to attach an pull requests to.

Thanks again for your help with this project.

Sure. I will get started on that

[pacharanero]

Review: is this still needed? Yes - and the current approach is the right one

Short answer to the question in the title: yes, this still needs addressing. Issue #53 is still live behaviour on live today. I reproduced it just now against the current package:

Input	mid_parental_height() on live today
Mother 10 cm (impossible)	silently returns 101.5
Father 300 cm (impossible)	silently returns 236.5
Mother -50 cm	silently returns 58.5
Invalid sex string	silently returns the female formula value
None maternal height	TypeError: unsupported operand type(s) for +: 'NoneType' and 'int' - confusing and far from the cause
String maternal height "151"	TypeError: can only concatenate str (not "int") to str

And mid_parental_height_z() happily returns SDS values (-6.27, 4.25) for physically impossible parents. The library-level functions are the foundation the API server builds on - the server does its own request validation, but every other consumer (notebooks, research code, downstream libraries) hits these functions directly, so the package does need its own guards. I agree with the principle stated in the PR description.

What this PR does well

• Uses the existing ±8 SDS error cut-offs (MINIMUM_HEIGHT_WEIGHT_OFC_ERROR_SDS / MAXIMUM_HEIGHT_WEIGHT_OFC_ERROR_SDS from validation_constants.py, from the issue Api BMI response not returning any errors for invalid measurements #32 discussion) instead of inventing new absolute cm thresholds. That is the right call: the SDS approach is reference-aware and sex-aware, whereas a single MAXIMUM_PARENTAL_HEIGHT_CM = 260 would be identical for mothers and fathers regardless of reference. The ±8 SDS range corresponds to roughly 115-212 cm for adult women and 122-233 cm for adult men on UK-WHO, which is physiologically sensible.
• Validates types (None, bool, non-numeric, non-finite) before they reach the arithmetic, converting confusing distant TypeErrors into clear ValueErrors at the actual cause.
• Boundary tests generated via measurement_from_sds() (deriving test heights from the requested SDS rather than hard-coding cm values) - robust to future reference-data changes. Good technique.
• Docstring and message quality are good.

Why it has not merged cleanly, and what I suggest

The branch is from 2026-01-25, before a lot of recent work, and is now in mergeable_state: dirty. Since it was opened:

1. The notebook execution CI (R2) now runs Quickstart.ipynb headlessly, so the version-stamp output-cell change in this diff conflicts with our current execution-driven updates - that hunk should be dropped.
2. The QA branch (PR test: establish code quality and QA gates #106) added test_public_api_contract.py, which pins the public signature mid_parental_height_z(maternal_height, paternal_height, reference) - the validation additions here preserve that signature, which is good, but the branch needs a rebase so the contract tests run against it.
3. The deprecated MINIMUM_PARENTAL_HEIGHT_CM comment change is fine, but if the SDS cut-offs fully supersede the cm constants, a follow-up question for maintainers is whether the cm constants should be removed entirely (that touches the API server's schemas/request_validation_classes.py, so it is a cross-repo decision - out of scope here).

Given the rebase conflicts are trivial (essentially just the notebook cell) and the design (SDS cut-offs) is better than the absolute-cm alternative I would otherwise have suggested, I am in favour of this landing after a rebase. Alternatively, if the original contributor is no longer around to rebase, the changes are small enough to be re-applied on a fresh branch with the same test suite - with attribution.

One substantive review note for the rebase: mid_parental_height() hard-codes reference=UK_WHO when validating via _validated_parental_height_z_scores(), while mid_parental_height_z() accepts any reference in REFERENCES. But sds_for_measurement() will itself reject unsupported reference/measurement combinations (e.g. trisomy-21 at adult age) with its own LookupError. Worth deciding whether the ±8 SDS validation in mid_parental_height_z should apply for all supported references (it does - fine) and whether the ValueError for invalid reference should match the wording used elsewhere in the package.

Also worth noting for the maintainer decision: raising for ±8 SDS is a behaviour change for any existing caller that deliberately calculates mid-parental heights outside those bounds (e.g. research on extreme stature). The cut-offs match the API server's existing request validation, so server behaviour is unchanged - only direct library callers see new exceptions. That is consistent with how the rest of the package treats impossible values (issue #51's raise-not-return work), so I support it.

Verdict

Still needed, sound design, tests are well-constructed. Recommend: rebase onto live, drop the notebook output-cell hunk, keep everything else, and re-run CI. Happy to review the rebased version promptly.

[pacharanero]

This has now been recreated from current live as #107, preserving FibrinLab <fibrinlab@gmail.com> as the implementation commit author and crediting Dr Akanimoh Osutuk explicitly in the PR description. The recreation keeps the SDS-based validation and tests, omits the stale notebook runtime output and unused absolute maximum constant, and adds non-finite-value coverage.

The Digital Growth Charts documentation attribution is tracked in rcpch/digital-growth-charts-documentation#184.

Closing this branch as superseded by #107. Thank you again @FibrinLab for the contribution.