Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements functionality for a prevalence option to enable the use of CCW-TMLE (Covariate-Conditional Weighted TMLE). The implementation adds prevalence as a parameter throughout the estimation pipeline while updating type names to match current TMLE.jl conventions.
- Adds
prevalenceparameter support to the CLI, runner, and model registry - Updates estimator type names from deprecated forms (TMLEE/OSE) to current ones (Tmle/Ose)
- Includes comprehensive test coverage for prevalence functionality with binary outcomes
Reviewed Changes
Copilot reviewed 22 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli.jl | Adds prevalence command-line argument |
| src/runner.jl | Integrates prevalence parameter into Runner and estimation functions |
| src/models/registry.jl | Updates model constructors to use current type names and pass prevalence |
| src/models/glmnet.jl | Adds weights parameter support to GLMNet fitting |
| test/runner.jl | Adds comprehensive test for prevalence functionality |
| test/testutils.jl | Adds binary-only dataset generation and configuration utilities |
| Various config files | Updates deprecated type names to current TMLE.jl conventions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
… LogisiticClassifier
|
Hi @olivierlabayle I have revisited the CLI to remove the |
|
Thanks @joshua-slaughter, I'll have a look at this before the end of the week. Have you tested this new implementation within the TARGENE pipeline? Does it cause any downstream issues that need versions to be propagated, i.e. in TargeneCore? |
|
It has required slight changes to TargeneCore.jl which I have implemented here https://github.com/TARGENE/TargeneCore.jl/tree/generate_outputs_update Beyond this there are some issues in targene-pipeline with the simulation workflows which you can view on the prevalence-parameter branch. |
olivierlabayle
left a comment
There was a problem hiding this comment.
@joshua-slaughter thanks for the good work. I've left a few minor comments that need to be addressed.
joshua-slaughter
left a comment
There was a problem hiding this comment.
Any reason for having two times what seems to be the same test with a different model? Probably drop one if not.
This was really a sanity check for me to see if the custom models were accepting weights currently in the full implementation. Would be okay with removing either.
|
Hi @olivierlabayle , I addressed your comments. Hopefully it is ready to merge, something seems to be wrong between versions. |
olivierlabayle
left a comment
There was a problem hiding this comment.
Thx Josh, just a few more things which shouldn't take long to fix. For the version problem I am pretty sure the issue comes from loading the Julia estimator file. I was pretty certain this would fire back at some point. So 1.12 is out of scope for now if we want to support that feature or we have to find a way to keep doing it. In the meantime we can change the version in CI.yml to :
version:
- '1.11' instead of '1' here (note that 1.11 is also the version used in the docker image
- '1.10'
|
Everything has been addressed! Should be ready to merge and up the version |
Adding functionality for the input of a
prevalenceoption to allow for the use of CCW-TMLE. Notable issues in implementation:MethodError.