Fix PaccMann integration - #447
Open
tereshchuk1 wants to merge 18 commits into
Open
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #447 +/- ##
===============================================
+ Coverage 80.34% 85.48% +5.13%
===============================================
Files 101 123 +22
Lines 8171 10119 +1948
===============================================
+ Hits 6565 8650 +2085
+ Misses 1606 1469 -137 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
|
Nice, thanks for the fix! I still see some issues: 1.) early_stopping is set to true, but the early stopping set is not used. Could you check if they use early stopping validation in the original implementation and either set it to false if they don't, or implement it?
3.) can you add the model to the documentation? |
…nfidence=...), _associate_language, utils/interpret.py, vendored pytoda) and unused gene_projection/smiles_projection/alpha_projection helpers in utils/layers.py
…for the TOY test, so the model no longer requires the gene_list_paccmann_network_prop panel on toy data
The original implementation (PaccMann/paccmann_predictor) trains for a fixed number of epochs and checkpoints the model whenever the loss on a held-out set improves, using that checkpoint as the final model. Follow the same procedure with the early stopping set: evaluate it after every epoch, keep the weights of the best epoch, and restore them at the end. Training still runs the full epoch budget, since the original does not terminate early either. Also deduplicate the input encoding shared by the validation loader and predict() into a _encode_inputs helper.
Tokenization: the wrapper split SMILES by character, so multi-character atoms fell apart -- 'Cl' became C+l and 'Br' became B+r, colliding with carbon and with boron (Bortezomib really contains boron), and bracket atoms such as '[C@@h]' or '[Pt+2]' were shredded into their individual characters. Use the atom-level regex from pytoda.smiles.processing, which is what the original implementation tokenizes with. Batch of size 1: torch.squeeze in the context attention layer also dropped the batch dimension, so predict() crashed whenever the row count left a trailing batch of one, and training hit the same in the batch norm layers. Squeeze only the last dimension, and drop a trailing single-sample training batch. The original implementation always sets drop_last=True; dropping only a size-1 batch keeps training sets smaller than one batch usable. Hyperparameters: raise smiles_padding_length 128 -> 512 and epochs 3 -> 10, the values from the original paccmann_v2_params.json. At 128 tokens, 13 CTRPv2 drugs were silently truncated; at 512 no drug in any bundled dataset is. Also remove six symbols that were defined but referenced nowhere: dense_attention_layer, to_np, attention_list_to_matrix, Unsqueeze, RNN_CELL_FACTORY and OPTIMIZER_FACTORY.
The original implementation re-randomizes every SMILES string on each access, so a drug is seen through a different but chemically identical SMILES in every epoch. With only a few hundred distinct drugs per dataset this is the main regularizer on the drug modality, and it was missing here. Each distinct drug gets a bank of variants built once with RDKit by re-serializing the molecule from a shuffled atom order; training then draws one variant per row per batch. The dataset stores the drug index instead of the encoded SMILES, so no per-epoch tensor is materialized. The vocabulary is built over all variants, otherwise their tokens would encode as unknown. Enabled by default via the augment_smiles hyperparameter. RDKit is a new optional dependency behind the paccmann extra; when it is missing, training warns and falls back to the unaugmented SMILES rather than failing.
Comments and code carried over from the original repository that never applied here: - Eight '# yapf: disable' directives. The original repository formats with yapf, drevalpy formats with black, which ignores them; black had already reformatted several of the lines they were attached to. - The prediction_dict returned by forward(). It held attention weights and IC50 variants 'to ease postprocessing' in the original analysis pipeline, but the wrapper always discarded it. It was still built on every evaluation batch, so removing it also drops two torch.cat calls per batch from predict(). forward() now returns the predictions alone. - The min_max_scaling branch with IC50_max/IC50_min and get_log_molar. It only activates when drug_sensitivity_processing_parameters is passed, which the wrapper never does, so it could not run. DrEval scales responses itself. - The 2128 default for number_of_genes, the gene panel size of the original implementation. Both construction paths pass the real count, so requiring it turns a stale magic number into a clear error. The attention weights remain available in the git history and upstream should interpretability ever be wired up.
Contributor
@PascalIversen why wouldn't we want to use early stopping no matter the original publication? |
JudithBernett
requested changes
Aug 10, 2026
…ing and dropping upstream cruft - consolidate paccmann_network_v2.py and the utils/ subpackage into a single network.py, with a typed PaccMannConfig replacing the raw hyperparameter dict threaded through the network - drop unused activation/loss factories and other dead configurability inherited from upstream - replace OrderedDict-of-named-Sequential blocks with plain nn.Module/nn.Sequential blocks - add patience-based early stopping, matching the rest of the models in this repo - simplify SMILES-array normalization and update docs accordingly
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
5 participants
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.
This PR fixes the PaccMann implementation originally submitted in #388 by @gretag04, adapting it to work within the drevalpy framework.
Bug fixes
pytodaimports:pytodais not a drevalpy dependency. The only code that needed it was the confidence-estimation path, which has since been removed entirely (see below), so neither the import nor the vendored stub remains.drug_smiles.csv; the huge fingerprint bit-string columns are skipped rather than loaded and dropped, since pandas' numeric type inference on them can raise anOverflowError.Review follow-ups
early_stoppingis nowFalse. The wrapper does not train against an early-stopping split, so it no longer requests one.forward(confidence=...)uncertainty path,_associate_language,utils/interpret.py, the vendoredpytodastub, and the unusedgene_projection/smiles_projectionhelpers inutils/layers.py.paccmann_v2.pytopaccmann_network_v2.py, to make clear thatpaccmann.pyis the DrEval wrapper and that this file holds the network it uses.predict()now runs batch-wise through aDataLoaderinstead of pushing all inputs through the network at once.self.log_hyperparameters(hyperparameters)inbuild_modelfor Weights & Biases logging.gene_listis now a hyperparameter, defaulting to thegene_list_paccmann_network_proppanel that ships with the repo. The TOY test sets it toNone, so the toy data does not need that panel.docs/drevalpy.models.PaccMann.rst, wired into the models toctree) and a row in the model overview table indocs/usage.rst.