Repository navigation
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## 1.9.0 #448 +/- ##
==========================================
- Coverage 52.77% 52.63% -0.14%
==========================================
Files 38 38
Lines 12177 12305 +128
==========================================
+ Hits 6426 6477 +51
- Misses 5751 5828 +77
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The requirements file is invalid, k-fold diagnostics reverse training and validation semantics, and the splitter signature breaks positional callers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds k-fold cross-validation diagnostics and ensures final RF/XGBoost test metrics reflect the saved retrained model.
Changes:
- Adds k-fold split plotting and tutorial data.
- Recomputes test metrics after final-model retraining.
- Updates splitter annotations and CPU dependencies.
File summaries
| File | Description |
|---|---|
pip/cpu_requirements.txt |
Updates TensorFlow dependency. |
atomsci/ddm/utils/compare_splits_plots.py |
Adds k-fold diagnostics and plotting. |
atomsci/ddm/pipeline/MultitaskScaffoldSplit.py |
Updates splitter type annotations. |
atomsci/ddm/pipeline/model_wrapper.py |
Evaluates final retrained RF/XGBoost models. |
atomsci/ddm/examples/tutorials/dataset/SLC6A3_Ki_curated_5_fold_cv_scaffold_e5ae743e-485d-42a6-a1cd-482a88b4d0df.csv |
Provides a five-fold tutorial split. |
Review details
- Files reviewed: 5/7 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| train_dir: Optional[str] = None, | ||
| valid_dir: Optional[str] = None, | ||
| test_dir: Optional[str] = None, |
There was a problem hiding this comment.
go ahead and just move these back where they were. it's a small change and not worth the possible breaking of backwards compatibility @ekchun
| train_ids = split_df.loc[ | ||
| tv_mask & (split_df["fold"] == fold_idx), "cmpd_id"].values |
| self.n_folds = None | ||
| self.fold_dists = None | ||
|
|
||
| def _init_kfold(self): |
|
|
||
| for fold_idx in fold_values: | ||
| train_ids = split_df.loc[ | ||
| tv_mask & (split_df["fold"] == fold_idx), "cmpd_id"].values |
There was a problem hiding this comment.
is this correct? it's assigning the train IDs as a single fold - wouldn't those be the validation IDs and the train IDs would be everything but that fold?
There was a problem hiding this comment.
i guess maybe it's constructed differently at the end and this is just pairwise distances for test to that individual fold. but calling it train IDs seems misleading.
There was a problem hiding this comment.
confirmed in model_datasets.py (load_presplit_dataset), fold == f is the validation fold and fold != f is that round’s training set... so train_ids was incorrectly selecting validation sets. resolved and updated the docstring to match
|
|
||
| # TensorFlow CPU; compatible with DeepChem 2.8 | ||
| tensorflow-cpu~=2.14.0 | ||
| tensorflow~=2.14.0 |
There was a problem hiding this comment.
this shouldn't be changed in this PR - revert
|
|
||
| pytest | ||
| ipykernel | ||
| ipykernel No newline at end of file |
There was a problem hiding this comment.
could be expanded: if self.is_kfold: hue = 'fold' else hue = subset
|
|
||
| pyplot.close() | ||
|
|
||
| def subset_frac_plot(self, dist_path=''): |
There was a problem hiding this comment.
could also be expanded - feature request probably not required right now
There was a problem hiding this comment.
expanded but we can review
| if len(dist_path) > 0: | ||
| save_figure(dist_path+'_frac_box') | ||
|
|
||
| def make_all_plots(self, dist_path=''): |
There was a problem hiding this comment.
i think this one should be expanded. logically if we're calling make_all_plots it should gracefully handle kfold or train/valid/test and it reduces burden on the user to manage which splitting stream they're using. i think for now you can just make two branches here: if is_kfold then dist_hist_kfold_plot and maybe the umap else: everything else already there
feature request would be to expand the fractions plots to handle folds as well and then mirror that in this function too
|
|
||
| return parser.parse_args() | ||
|
|
||
| if __name__ == '__main__': |
There was a problem hiding this comment.
if you don't expand make_all_plots then this main block needs a different route for k_fold splits. i think it's easier to just expand make all plots.
There was a problem hiding this comment.
expanding make all plots
There was a problem hiding this comment.
Hi @ekchun I added inline comments where appropriate and include additional information here:
Inline comments
Mostly, expand the make_all_plots function to cover k_fold plotting instead of just standard plotting.
The tutorial notebook
I updated some descriptions and a path to be relative. Once you update the plotting I recommend updating the plotting section in the notebook to use the "plot all" function too.
The requirements.txt
Changing this file can have a lot of unintended consequences and deserves its own PR with thorough testing so just revert the changes you have here.
Copilot
I asked copilot to review and it had some minor changes to suggest. Go ahead and implement those too.
CodeCov
This analysis checks to see if the new code is covered by our test suite. Once all the new changes are in, we should add tests based on its report (use claude or whatever). This should be the last step when we're done changing all the code
Ruff linter
These are code-quality syntax checks. Go ahead and review the list and fix them - there's only a couple. You can investigate how to install ruff and then auto-fix them with the --fix command in ruff, or just fix them by hand.
fix zenodo badge with different svg
No description provided.