Conversation
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
v2.2.0 expands scoring, repeated-CV boundary analysis, classification handling, reporting, and model evaluation workflows.
Changes:
- Independent Interpolation and Boundary robustness scoring.
- New VERIFY diagnostics, stability metrics, SHAP optimizations, and split handling.
- Added
--all_modelsreporting and EVALUATE smoke coverage.
File summaries
| File | Description |
|---|---|
| tests/test_easyrob.py | Updated as part of this pull request. |
| tests/test_6evaluate.py | Updated as part of this pull request. |
| tests/test_6evaluate.future | Updated as part of this pull request. |
| tests/test_5aqme_n_full.py | Updated as part of this pull request. |
| tests/test_4predict.py | Updated as part of this pull request. |
| tests/test_3verify.py | Updated as part of this pull request. |
| tests/test_2generate.py | Updated as part of this pull request. |
| tests/test_1curate.py | Updated as part of this pull request. |
| setup.py | Updated as part of this pull request. |
| robert/verify.py | Updated as part of this pull request. |
| robert/robert.py | Updated as part of this pull request. |
| robert/report_utils.py | Updated as part of this pull request. |
| robert/predict.py | Updated as part of this pull request. |
| robert/predict_utils.py | Updated as part of this pull request. |
| robert/gui_easyrob/utils/predictions_utils.py | Updated as part of this pull request. |
| robert/gui_easyrob/tutorials/overview.md | Updated as part of this pull request. |
| robert/gui_easyrob/tabs/predictions.py | Updated as part of this pull request. |
| robert/gui_easyrob/tabs/advanced_options.py | Updated as part of this pull request. |
| robert/gui_easyrob/main/window.py | Updated as part of this pull request. |
| robert/generate.py | Updated as part of this pull request. |
| robert/generate_utils.py | Updated as part of this pull request. |
| robert/evaluate.py | Updated as part of this pull request. |
| robert/curate.py | Updated as part of this pull request. |
| robert/argument_parser.py | Updated as part of this pull request. |
| robert/aqme.py | Updated as part of this pull request. |
| robert/api.py | Updated as part of this pull request. |
| README.md | Updated as part of this pull request. |
| docs/Tutorials/overview.rst | Updated as part of this pull request. |
| docs/Report/score.rst | Updated as part of this pull request. |
| docs/Report/images/spearman_rank_diagram.svg | Updated as part of this pull request. |
| docs/Report/images/sorted_low_high_diagram.svg | Updated as part of this pull request. |
| docs/Report/images/leverage_diagram.svg | Updated as part of this pull request. |
| docs/Report/images/degradation_ratio_diagram.svg | Updated as part of this pull request. |
| docs/Report/images/crossing_rate_diagram.svg | Updated as part of this pull request. |
| docs/README.rst | Updated as part of this pull request. |
| docs/Misc/versions.rst | Updated as part of this pull request. |
| .circleci/config.yml | Updated as part of this pull request. |
Review details
Suppressed comments (9)
robert/gui_easyrob/utils/predictions_utils.py:155
- The new report puts Interpolation on the left and Boundary robustness on the right inside both suffix-specific PDFs, so
model_keyis not a valid left/right selector here. As written, the No_PFI extraction selects the Interpolation column while the PFI extraction selects the Boundary column, causing the dashboard score/fragment to be wrong even if the correct suffix PDF is supplied; select the region by metric instead.
robert/predict_utils.py:271 - The classification stability facet is defined per CV repeat, but
y_pred_test_allcontains one prediction for every fold of every repeat and this code compares all raw fold predictions directly with the majority vote. That measures fold-level disagreement rather than repeat-level disagreement and can change the score; aggregate each repeat's fold predictions before computing the disagreement rate.
robert/predict_utils.py:254 - These stability diagnostics are computed using
model_data['repeat_kfolds'], but the output label is hardcoded to “10 repeats” (the same label is used for the train agreement and MCC lines below). A run with--repeat_kfolds 5, for example, reports the wrong methodology; use the configured repeat count in every stability label and in the corresponding parser.
robert/predict_utils.py:249 y_pred_train_allcontains one prediction per point for every fold of every repeat, so its columns arerepeat_kfolds * kfold, not one column per repeat. Computing RMSE/MCC across those columns treats individual fold predictions as separate repeats and does not measure the stated per-repeat stability; group each set of folds into one prediction per repeat before calculating the coefficient of variation.
robert/predict_utils.py:275- The train+validation classification facet has the same repeat-boundary problem:
y_pred_train_allcontains fold-level predictions, but this computes one agreement rate over all folds against a single vote across all repeats. That is not the specified repeat-to-repeat agreement; aggregate each repeat's out-of-fold predictions before comparing with the overall majority vote.
robert/report_utils.py:1264 - The new item is described as comparing each fold's validation RMSE with that same fold's training RMSE, but this loop finds one aggregate
Train fit (in-fold)line and divides the aggregate CV RMSE by the aggregate in-fold RMSE. Per-fold gaps are never retained, so a badly overfit fold can be hidden by averaging; compute and score the fold-level ratios.
robert/utils.py:1642 StratifiedShuffleSplitonly preserves at least one sample per class in the train/test split; it does not ensure that the resulting training set has at leastkfoldsamples per class for the downstreamStratifiedKFold. A small balanced classification dataset (for example, 5 samples per class with the default 4-point test set) leaves fewer than five members per class, causing warnings and folds without a class during every CV/BO run. The split or kfold value needs validation/adjustment before proceeding.
robert/utils.py:3288- For multiclass classifiers, TreeExplainer returns one explanation per class, but this unconditionally keeps only the last class. Since this release now supports classification splits with three or more classes, the report's unlabeled SHAP plot and min/max summary can describe an arbitrary class rather than the model's predictions; handle multiclass outputs explicitly (aggregate or label/render per-class values) instead of treating the last class as universally positive.
robert/utils.py:3262 - For RF/GB classifiers, the previous generic explainer wrapped
loaded_model.predict(hard class labels), whileTreeExplainer(loaded_model)explains the estimator's continuous tree output/probability and the code selects the positive-class channel. Thus the classification SHAP values and plots change semantically, despite the PR describing this as the same result; explicitly choose and document a matching output (for examplepredict_probaif probabilities are intended) or preserve the old predictor target.
- Files reviewed: 34/59 changed files
- Comments generated: 14
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self.args.log.write(f"\nx WARNING! No results were found for these models, they will be skipped in the heatmap: {missing_models}") | ||
| df_cols = [model for model in df_cols if model not in missing_models] | ||
|
|
||
| csv_df = csv_df[df_cols] |
| """Given the path to a selected file, return the corresponding ROBERT_report.pdf file.""" | ||
| return Path(selected_file_path).parent / "ROBERT_report.pdf" | ||
| """Given the path to a selected file, return the corresponding ROBERT report PDF file.""" | ||
| return Path(selected_file_path).parent / "ROBERT_report_No_PFI.pdf" |
| score_dat,data_score = self.print_score(dat_files,pred_type,eval_only,spacing_PFI) | ||
| report_html += score_dat | ||
| # generate one PDF per model (No PFI / PFI) | ||
| for suffix in ['No PFI', 'PFI']: |
Comment on lines
+1268
to
+1271
| if data_score[f'scaled_rmse_trainfit_{suffix}'] > 0: | ||
| factor_trainfit = data_score[f'scaled_rmse_cv_{suffix}'] / data_score[f'scaled_rmse_trainfit_{suffix}'] | ||
| else: | ||
| factor_trainfit = 0 |
Comment on lines
+1549
to
+1552
| if crossing_rate < 0.2: | ||
| penalty_crossing -= 2 | ||
| elif crossing_rate < 0.5: | ||
| penalty_crossing -= 1 |
| params_df = pd.read_csv(file_param, encoding='utf-8') | ||
| params_dict = pd_to_dict(params_df) # (using a dict to keep the same format of load_model) | ||
| # set the parameters for the ML model | ||
| params_dir = f'{self.args.params_dir}/{"_".join(suffix.split())}' |
Comment on lines
+1151
to
+1154
| if scaled_rmse_bottom80 and scaled_rmse_top80: | ||
| data_score[f'degradation_ratio_high_{suffix}'] = scaled_high / scaled_rmse_bottom80 | ||
| data_score[f'degradation_ratio_low_{suffix}'] = scaled_low / scaled_rmse_top80 | ||
| data_score[f'degradation_score_{suffix}'] = int(data_score[f'degradation_ratio_low_{suffix}'] <= 1.5) + int(data_score[f'degradation_ratio_high_{suffix}'] <= 1.5) |
Comment on lines
+1251
to
+1254
| if data_score[f'scaled_rmse_cv_{suffix}'] > 0: | ||
| data_score[f'factor_scaled_rmse_{suffix}'] = data_score[f'scaled_rmse_test_{suffix}'] / data_score[f'scaled_rmse_cv_{suffix}'] | ||
| else: | ||
| data_score[f'factor_scaled_rmse_{suffix}'] = 0 |
Comment on lines
+2261
to
+2269
| # in-fold training fit (each point is predicted by the fold(s) where it was part of | ||
| # the training portion, i.e. NOT held out) - used for the train-vs-validation gap score | ||
| y_train_infold_pred = [] | ||
| for y_val_trin in y_pred_global_train_infold: | ||
| if model_data['type'].lower() == 'reg': | ||
| y_train_infold_pred.append(np.mean(y_val_trin)) | ||
| elif model_data['type'].lower() == 'clas': | ||
| y_train_infold_pred.append(_classification_vote(y_val_trin)) | ||
| Xy_data['y_pred_train_infold'] = y_train_infold_pred |
Comment on lines
+2930
to
+2931
| ax = sb.heatmap(csv_df, annot=True, linewidth=1, cmap=cmap_blues_75_percent_512, | ||
| vmin=0, vmax=10, cbar_kws={'label': 'Score (0-10)'}, mask=csv_df.isnull()) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #85 +/- ##
==========================================
- Coverage 57.23% 55.77% -1.47%
==========================================
Files 32 32
Lines 8804 9492 +688
==========================================
+ Hits 5039 5294 +255
- Misses 3765 4198 +433 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Interpolation and Boundary robustness are now independent scores
Boundary robustness (formerly "Extrapolation" - renamed since the metric evaluates
robustness at the edges of the observed y-range via repeated CV, not genuine
out-of-domain extrapolation) columns, with the right-hand column mostly duplicating or
leaving blank its sub-metrics. Both columns now have their own independent 0-10 score,
their own score icon, and five dedicated sub-metrics each (0-2 points each), scored and
explained separately
Boundary robustness now uses the same repeated-CV methodology as Interpolation
sorted-CV folds, not statistical quartiles) predictions and the Applicability Domain
analysis used to come from a single train-once/test-once fit. They are now computed with
the same 10x repeated 5-fold CV that Interpolation's "10x 5-fold CV"/"test" plot already
used, scoped to each extreme's own 80/20 partition of the sorted dataset. This makes
Boundary robustness's RMSE directly comparable to Interpolation's RMSE (both are averages
over the same kind of repeated procedure, instead of one being averaged and the other a
single noisy draw), and it fixed a real bug where the "Scaled RMSE (Low/High, sorted CV)"
text shown in the report was still silently reading VERIFY's old single-pass values even
after the plots themselves had already switched to the new repeated-CV predictions
Boundary robustness sub-metrics redesigned
thresholds), now penalized by how many of the extreme predictions actually cross beyond
the training range ("real range extension" / crossing rate) instead of by Spearman rank
own dedicated, independently-scored item instead of only acting as an internal penalty
inside the RMSE items above
RMSE of its own "remaining 80%" baseline from the same sorted-CV run. Low and High are
now scored independently (+1 each, <=1.5x tiering) instead of taking the worse (max) of
the two sides, so a badly-degraded side is no longer invisible just because the other
side happens to be fine
also fed by the repeated-CV predictions instead of a single fit
whole sorted dataset as context (the local, per-extreme Spearman is already scored in
item 3), without double-counting rank that other items already score
into the column total)
New Interpolation item: train vs. validation gap
against that same fold's own in-fold training RMSE (scaled RMSE (test) <= 1.25x train:
+2, <= 1.5x: +1), to catch a model that fits its own training folds much better than it
generalizes to their held-out validation fold - a form of overfitting that a validation-only
metric can miss. The existing "CV vs test" comparison (now item 5) was retitled "CV vs
test consistency" to disambiguate it from this new item, since both compare two error
values but on different axes (fold-internal train/validation vs. overall CV/test)
Interpolation item 6 redesigned into three stability facets
facets (each scored 0-2, final score is their average, rounded): (a) SD of the
repeated-CV test-set predictions (unchanged), (b) SD of the out-of-fold
train+validation predictions (new), (c) coefficient of variation of the aggregate RMSE
across the 10 CV repeats (new) - a dataset-wide view of the same "how much does this
depend on the random split" question that (a)/(b) ask per-point
Report PDF rendering fixes
applicability domain, Williams plot, SD plots): some showed no title, others a
half-cut one, depending on each image's exact aspect ratio. The saved PNG files in
PREDICT/VERIFY keep their full title as before; the report's image containers now use a
height measured precisely from each image type so the title is cropped out cleanly and
consistently instead of only in some plots
few descriptors, e.g. only 1 of 4 cells visible with 2 descriptors): masking the upper
triangle left the always-fully-masked first row and last column as still-valid (if
invisible) axes content, which
bbox_inches='tight'does not crop away. The heatmapnow trims that empty row/column from the plot only; the function's returned correlation
matrix is unchanged (kept full-size, since PREDICT's correlation-pair detection indexes
into it positionally and would silently miscount pairs if it were trimmed too)
VERIFY: new flawed-model test - cluster mean baseline
y_mean/y_shuffle/onehot: abaseline that clusters the descriptors into two groups with KMeans and predicts each
point using its own cluster's training-y mean, evaluated out-of-fold with the same
repeated 10x 5-fold CV used for the real model. This specifically targets bimodal/
structured datasets, where a trivial "which cluster am I in" rule can otherwise reach
deceptively good RMSE and go undetected by the existing three tests
Bayesian Optimization: integer hyperparameters no longer reported with decimals
of queued lazily), and every suggestion from the optimizer now has its integer-type
hyperparameters (e.g.
n_estimators,max_depth) rounded before being registered.The model itself was always fit with the correctly rounded value, but the reported/saved
"best params" could previously show misleading decimals for these hyperparameters
Removed obsolete LOOCV documentation
--kfoldhelp text described a "for databases with less than 50 points, do LOOCV"auto-behavior that was never implemented (
kfoldhas always simply defaulted to 5,regardless of dataset size). The help text now matches the actual behavior
VERIFY module: fixed a systematic bias in the one-hot test
1". For any strictly-positive continuous descriptor with no literal zeros (e.g. a
molecular weight), this silently collapsed it into a useless constant column instead of
a genuine two-way split, which could make the test fail regardless of whether the model
was actually flawed. Continuous descriptors are now binarized with an unsupervised
per-descriptor median split (value >= median -> 1, else -> 0), which does not require
literal zeros and does not look at the target value (keeping the test an unbiased
sanity check)
being median-split: applying the median split to an imbalanced already-binary
descriptor (its minority class under 50%) reproduces the same collapse-to-constant
failure, just triggered by class imbalance instead of by missing literal zeros. This is
handled per descriptor, so datasets mixing binary and continuous descriptors are scored
correctly instead of only checking the dataset as a whole
after the split; that single descriptor is now dropped from the one-hot matrix instead
of letting it invalidate the whole test
already has 2 or fewer unique values, or when every descriptor collapses to a constant
after binarization, since there is no continuous variation left to destroy and the test
would otherwise always report a spurious failure
Bayesian Optimization: hyperparameter search space revised
~10,000 at most) rather than scikit-learn's generic defaults, which assume much larger
samples. RF/GB's
max_depthare no longer shared: GB (boosting) now searches ashallower 2-10 range instead of RF's 5-20, matching scikit-learn's own default of 3 for
gradient boosting. NN's
alphalower bound widened down to scikit-learn's own default(0.0001), which the previous 0.01 floor excluded from the search entirely
fitted model: GB's
validation_fraction(only used ifn_iter_no_changeis set,which ROBERT never does, so scikit-learn silently ignores it), RF/GB's
min_weight_fraction_leaf(constrains leaf size by weighted sample fraction, butROBERT never passes
sample_weighttofit(), so with implicit uniform weights thisis a redundant, weaker echo of
min_samples_leaf/min_samples_split, alreadysearched), and NN's
tol(affects when the solver stops, not the quality of the fit)n_estimatorsin stepsof 10, NN's
max_iterin steps of 25) instead of every single integer, so each BOevaluation explores meaningfully different configurations instead of two iterations
landing on near-identical values (e.g. 71 vs. 72 trees)
New --all_models option: full VERIFY/PREDICT/REPORT for every screened model
VERIFY/PREDICT/REPORT only ever ran on the single best one. With
--all_models True,every model GENERATE screened gets its own full VERIFY -> PREDICT -> REPORT pass and its
own PDF (e.g.
ROBERT_report_RF_No_PFI.pdf,ROBERT_report_NN_PFI.pdf, ...), so theuser can compare and pick a model manually instead of only ever seeing the
auto-selected best one. Only this VERIFY/PREDICT/REPORT tail runs once per model - the
expensive BO search in GENERATE is unaffected, since it already computes all 4 models
regardless of this option
of GENERATE's raw combined-RMSE-per-model heatmap (only reflects the BO search
criterion), it shows each model's final Interpolation/Boundary robustness score (0-10), which
is a more informative basis for comparing models once VERIFY/PREDICT have finished
PREDICT: faster SHAP analysis for RF and GB
shap.Explainer(model.predict, X), i.e. through the genericpredict-function wrapper. This hides the tree structure from SHAP, so it fell back to the
Exact/Permutation explainer, which scales poorly with dataset size and descriptor count.
RF and GB now use
shap.TreeExplainer(model)directly, which computes the same exactShapley values through the polynomial-time TreeSHAP algorithm instead of the slow
generic fallback - same results, much faster on large datasets. NN and MVL are unaffected
(TreeSHAP doesn't apply to them)
TreeExplainerreturns SHAP values per class (an extra 3rd array dimension) instead ofthe plain per-descriptor 2D array the generic explainer returned, which broke both the
summary plot and the printed min/max SHAP values per descriptor. The last class (the
positive class in binary problems) is now selected right after computing SHAP values, so
plotting and the printed summary both consistently work on a 2D array again
REPORT: classification Interpolation score now reaches 10, same as regression
predictions, max 3 + the flawed-models penalty, always ≤ 0 + the MCC-difference item, max 2),
missing the two points regression gets from item 6 (prediction stability) because that item's
SD-based, %-of-y-range definition doesn't apply to a discrete label. Item 6 is now defined for
classification too, adapting the same three facets: (a)/(b) the disagreement rate between
individual CV repeats and the already majority-voted class (test set and out-of-fold
train+validation, respectively), and (c) the coefficient of variation of the per-repeat MCC
instead of the per-repeat RMSE - each scored 0-2 with the same ≤15%/≤25% thresholds used by
regression's facet (c), then averaged and rounded, same as regression. This was a stale
denominator bug in passing (the percentage-of-max used to look up the score tier was dividing
by 9 - a leftover from an assumption that the flawed-models item could contribute +1, which it
never does - before this fix corrected it to 8, now superseded by the real max of 10)
GENERATE/REPORT: --test_set 0 now works on its own, and the score is hidden when the
test set isn't the standard split
--test_set 0used to still get silently raised to 0.2 by the--auto_testsafetynet (needing
--auto_test Falseon top to actually take effect) even though anexplicit 0 is an unambiguous, deliberate "train on 100% of the data" choice, not a value
that needs the safety net - only small-but-nonzero values (a likely accident) still get
raised
(see the note in score.rst). With no internal test set at all (
--test_set 0, or anyother non-standard
--test_set), the "test set predictions" score component silentlycomputed as 0 (
NaN <= thresholdcomparisons are alwaysFalsein Python, so thisfailed silently, no crash and no warning), which could tank an otherwise good model's
score for a reason invisible in the PDF. Section A now shows a short explanatory notice
instead of a score in this case, the "Overall assessment" line shows "Not available"
instead of a verdict based on that score, and Section B (entirely a breakdown of the
score) is skipped - the rest of the report (SHAP, PFI, outliers, reproducibility, etc.)
is unaffected
GENERATE: fixed the STRATIFIED split for classification, and warn on split methods
that don't apply to classification
--split stratifiedused to cap the min/max target value and bin the remaining valuesinto quantiles via
pd.qcut()before stratifying - a regression-only approach (binninga continuous target). For classification this ignored the actual class labels entirely,
relying on
qcutaccidentally degenerating to ~2 bins for a binary target to produce apassably-balanced split; for 3+ classes it was blocked outright (fell back to RND). Now
classification stratifies directly on the class labels via
StratifiedShuffleSplit,which works for any number of classes
STRATIFIED split too:
StratifiedShuffleSplit.split()yields(train_idx, test_idx)in that order, but the code unpacked it as
for test_idx, _ in ..., taking the trainindices as if they were the test indices. This had been silently compensated by a second,
opposite bug (
test_size=(100 - size) / 100instead ofsize / 100, i.e. requestingthe complement), so the two bugs cancelled out and regression's STRATIFIED split ended up
correct anyway - fixed to compute both correctly on their own, without relying on that
accidental cancellation
y") that has no defined meaning for a discrete class label, regardless of class count -
choosing one of these for a classification run now logs a warning and falls back to
STRATIFIED (previously this fallback only existed for KN/STRATIFIED with 3+ classes, and
fell back to RND)
--split auto, or no--splitat all) changed fromRND to STRATIFIED - preserving class proportions in both train and test is standard
practice for classification, and a plain random split can, by chance, leave a class
under/over-represented, especially on small datasets