-
Notifications
You must be signed in to change notification settings - Fork 12
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 PatientLevelPredictionValidationModule & ModelTransferModule #161
base: v1.0-main
Are you sure you want to change the base?
Conversation
targetId = df$target_id[1], | ||
outcomeId = df$outcome_id[1], | ||
plpModelList = as.list(df$modelPath), | ||
restrictPlpDataSettings = jobContext$settings[[1]]$restrictPlpDataSettings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this here for @egillax - just wanted to note that the module's settings implies a list of lists for the validationComponentsList
but here we're only using the first element of the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's not correct. Thanks for catching that. I as well see some other issues, the createPatientLevelPredictionValidationModuleSpecifications
input shouldn't have a validationSettings
item in the list of lists and I should as well document what the function expects as input.
Would you accept a PR to this branch fixing those things ?
# ) | ||
|
||
# check the model locations are valid and apply model | ||
upperWorkDir <- dirname(jobContext$moduleExecutionSettings$workFolder) # AGS: NOTE - Using the "root" folder as the expection is that the ModelTransferModule output is here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking this to #21 since I'm unsure of the workflow between the ModelTransferModule and the PLP Validation module. In looking at the upperWorkDir
the assumption I see is that the ModelTransferModule is written to the "root" folder that holds the results/work sub folders. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I completely follow. The ModelTransferModule
should write the models to either results/work module subfolder in a directory models
. The current code writes it in the work subfolder. Then it writes a csv file in the results directory that has the paths to each individual model in the work directory.
So something like:
├── strategusOutput
│ ├── DatabaseMetaData
| ├── CohortGeneratorModule
│ ├── DatabaseMetaData
│ ├── ModelTransferModule
│ │ └── github_export.csv
├── strategusWork
│ ├── CohortGeneratorModule
│ ├── ModelTransferModule
│ │ └── models
│ │ └── model_github_PandemicPrediction_master
│ │ ├── model_1
│ │ ├── model_2
Since the downloaded models are the results of the ModelTransferModule
it could be argued they should be in the results subfolder instead (strategusOutput/ModelTransferModule/models
) . But I would defer to you which fits better in with Strategus.
In current code the upperWorkDir
should be the strategusWork
directory. And in line 61 modelSavelocation
should point to strategusWork/ModelTransferModule/models
and that's where the validation module looks for the models. I'm using sort(dir(upperWorkDir, pattern = 'ModelTransferModule'), decreasing = T)[1]
to identify the correct module since it used to have trailing underscores and numbers. If that's not the case anymore this can be simplified to a simple file.path
.
Hope that's clear!
Egill
Aims to migrate the PatientLevelPredictionValidationModule & ModelTransferModule to the new Strategus v1.0 format.
This PR is lacking test cases at the moment which I plan to add with some guidance from @egillax.