Skip to content

Add error handling for mid-parental-height functions#78

Open
FibrinLab wants to merge 2 commits intorcpch:livefrom
FibrinLab:add-mid-parental-height-validation
Open

Add error handling for mid-parental-height functions#78
FibrinLab wants to merge 2 commits intorcpch:livefrom
FibrinLab:add-mid-parental-height-validation

Conversation

@FibrinLab
Copy link

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

- Add MAXIMUM_PARENTAL_HEIGHT_CM constant (260 cm) to validation_constants.py
- Add comprehensive input validation to mid_parental_height() function:
  * Validate sex parameter (must be 'male' or 'female')
  * Validate maternal_height (None, type, range 50-260 cm)
  * Validate paternal_height (None, type, range 50-260 cm)
- Add comprehensive input validation to mid_parental_height_z() function:
  * Validate reference parameter
  * Validate maternal_height (None, type, range 50-260 cm)
  * Validate paternal_height (None, type, range 50-260 cm)
- Add 7 new test functions covering all validation scenarios
- All tests pass (22,767 total tests)

Fixes issue: mid-parental-height accepts impossible values
@mbarton mbarton self-assigned this Jan 26, 2026
@mbarton
Copy link
Member

mbarton commented Jan 26, 2026

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
Copy link
Member

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
Copy link
Author

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

@eatyourpeas
Copy link
Member

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
Copy link
Author

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
Copy link
Member

eatyourpeas commented Feb 21, 2026

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
Copy link
Author

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

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