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

Add new schema for static linear model and enforce model schemas #532

Merged
merged 6 commits into from
Sep 27, 2024

Conversation

alexdewar
Copy link
Contributor

This PR adds a new v2 schema for the static linear model, which adds the various trend-related fields. If you could check that the constraints I've chosen seem sensible (e.g. does anything need a max/min value property?) that would be great. Besides that, I don't think any of the other schemas need new versions yet (though see #531), but if I've missed any, please let me know!

I've added the code to enforce these schemas at model load time. I did a small amount of refactoring to facilitate this (mostly amalgamating the static and dynamic model loading functions).

Closes #529. Closes #530.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 3.92157% with 49 lines in your changes missing coverage. Please review.

Project coverage is 48.72%. Comparing base (9a6e7e3) to head (f69234c).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/HealthGPS.Input/model_parser.cpp 0.00% 49 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #532      +/-   ##
==========================================
- Coverage   48.82%   48.72%   -0.11%     
==========================================
  Files         164      164              
  Lines        7779     7795      +16     
  Branches     1059     1063       +4     
==========================================
  Hits         3798     3798              
- Misses       3972     3988      +16     
  Partials        9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jamesturner246 jamesturner246 left a comment

Choose a reason for hiding this comment

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

No, I think the schema seems sensible. Depending on if you want to, strictly speaking a number of the numbers cannot be negative. For instance stddevs, prevalence, but this covers the majority of it. Up to you.

Else LGTM 👍
Usual questions

Comment on lines +29 to +33
constexpr int DummyModelSchemaVersion = 1;
constexpr int EBHLMModelSchemaVersion = 1;
constexpr int KevinHallModelSchemaVersion = 1;
constexpr int HLMModelSchemaVersion = 1;
constexpr int StaticLinearModelSchemaVersion = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to worry about it for now, but will it be possible eventually to load older configs using older schemas? For instance by using the config's schema version directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's the plan. Atm I'm not requiring the $schema attribute, but we could do. I was just thinking that we could try to validate a file with the newest version of the schema and if that doesn't work, we try the next oldest etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, this isn't used anywhere right now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have said, but no, it's not used by HGPS. I do use static.json and dynamic.json for validating config files in healthgps-examples, but in HGPS I load the model schemas directly because you get clearer error messages that way.

@alexdewar alexdewar enabled auto-merge September 27, 2024 07:51
@alexdewar alexdewar merged commit 46c27c1 into main Sep 27, 2024
5 of 7 checks passed
@alexdewar alexdewar mentioned this pull request Sep 27, 2024
@alexdewar alexdewar deleted the model-schemas-v2 branch September 30, 2024 06:40
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.

Enforce model schemas when loading models Update model schemas to correspond to latest code
2 participants