Fix PaccMann integration - #447
Open
tereshchuk1 wants to merge 16 commits into
Open
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #447 +/- ##
===============================================
+ Coverage 80.34% 84.94% +4.59%
===============================================
Files 101 126 +25
Lines 8171 10010 +1839
===============================================
+ Hits 6565 8503 +1938
+ Misses 1606 1507 -99 ☔ 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? |
Collaborator
There was a problem hiding this comment.
can we rename the file paccmann_network_v2 (to make clear that paccmann.py is not hte first version, but uses this network)
Contributor
There was a problem hiding this comment.
I don't understand this, why are there versions?
…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
Comment on lines
+10
to
+25
| LOSS_FN_FACTORY = { | ||
| "mse": nn.MSELoss(), | ||
| "l1": nn.L1Loss(), | ||
| "mse_and_pearson": mse_cc_loss, | ||
| "pearson": correlation_coefficient_loss, | ||
| "binary_cross_entropy": nn.BCELoss(), | ||
| } | ||
|
|
||
| ACTIVATION_FN_FACTORY = { | ||
| "relu": nn.ReLU(), | ||
| "sigmoid": nn.Sigmoid(), | ||
| "selu": nn.SELU(), | ||
| "tanh": nn.Tanh(), | ||
| "lrelu": nn.LeakyReLU(), | ||
| "elu": nn.ELU(), | ||
| } |
Contributor
There was a problem hiding this comment.
Shouldn't we also tune some of this, then? Or if not, drop?
Contributor
There was a problem hiding this comment.
I don't understand this, why are there versions?
Comment on lines
+105
to
+112
| The original implementation (https://github.com/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. This wrapper follows the same procedure with the early stopping set: training always runs for | ||
| the full epoch budget and the weights of the best epoch are restored at the end. There is no patience-based | ||
| termination, since the original implementation does not stop early either. | ||
| """ | ||
|
|
||
| early_stopping = True |
Contributor
There was a problem hiding this comment.
I don't like that, I think all of our models should follow the same training procedure. If early stopping is set to true, it should mean the same for all the models
Comment on lines
+211
to
+228
| def _normalize_smiles_array(self, smiles_raw: np.ndarray) -> list[str]: | ||
| """Convert SMILES output from FeatureDataset into a list of strings. | ||
|
|
||
| :param smiles_raw: raw SMILES array | ||
| :return: list of SMILES strings | ||
| """ | ||
| smiles_raw = np.asarray(smiles_raw, dtype=object) | ||
|
|
||
| if smiles_raw.ndim == 2 and smiles_raw.shape[1] == 1: | ||
| smiles_raw = smiles_raw[:, 0] | ||
|
|
||
| smiles_list = [] | ||
| for smile in smiles_raw: | ||
| if smile is None: | ||
| smiles_list.append("") | ||
| else: | ||
| smiles_list.append(str(smile)) | ||
| return smiles_list |
Contributor
There was a problem hiding this comment.
Suggested change
| def _normalize_smiles_array(self, smiles_raw: np.ndarray) -> list[str]: | |
| """Convert SMILES output from FeatureDataset into a list of strings. | |
| :param smiles_raw: raw SMILES array | |
| :return: list of SMILES strings | |
| """ | |
| smiles_raw = np.asarray(smiles_raw, dtype=object) | |
| if smiles_raw.ndim == 2 and smiles_raw.shape[1] == 1: | |
| smiles_raw = smiles_raw[:, 0] | |
| smiles_list = [] | |
| for smile in smiles_raw: | |
| if smile is None: | |
| smiles_list.append("") | |
| else: | |
| smiles_list.append(str(smile)) | |
| return smiles_list | |
| def _normalize_smiles_array(self, smiles_raw: np.ndarray) -> list[str]: | |
| """Convert SMILES output from FeatureDataset into a list of strings. | |
| :param smiles_raw: raw SMILES array | |
| :return: list of SMILES strings | |
| """ | |
| smiles_raw = np.asarray(smiles_raw, dtype=object).reshape(-1) | |
| return ["" if s is None else str(s) for s in smiles_raw] |
I'm confused about this function.
smiles_raw should already be an np.ndarray according to the function definition. Why do we need to cast it?
Why do we need this check? Shouldn't the smiles always have the same format because they are always constructed the same way?
This is so long
Comment on lines
+7
to
+22
| def get_device(): | ||
| """Return the active torch device. | ||
|
|
||
| :return: torch.device("cuda") if cuda is available otherwise torch.device("cpu") | ||
| """ | ||
| return torch.device("cuda" if cuda() else "cpu") | ||
|
|
||
|
|
||
| def cuda(): | ||
| """Check whether cuda is available. | ||
|
|
||
| :return: True if cuda is available otherwise False. | ||
| """ | ||
| return torch.cuda.is_available() | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
really unneccessary functions around basic torch functions, please remove
Comment on lines
+23
to
+52
| class Squeeze(nn.Module): | ||
| """Squeeze wrapper for nn.Sequential.""" | ||
|
|
||
| def forward(self, data): | ||
| """Squeeze the last dimension of the input tensor. | ||
|
|
||
| :param data: input tensor | ||
| :return: squeezed tensor | ||
| """ | ||
| return torch.squeeze(data, -1) | ||
|
|
||
|
|
||
| class Temperature(nn.Module): | ||
| """Temperature wrapper for nn.Sequential.""" | ||
|
|
||
| def __init__(self, temperature): | ||
| """Initialize the temperature wrapper. | ||
|
|
||
| :param temperature: Temperature value used for scaling. | ||
| """ | ||
| super().__init__() | ||
| self.temperature = temperature | ||
|
|
||
| def forward(self, data): | ||
| """Scale the input tensor by the temperature value. | ||
|
|
||
| :param data: input tensor | ||
| :return: scaled tensor | ||
| """ | ||
| return data / self.temperature |
Contributor
There was a problem hiding this comment.
also here: why do these need to be modules? these are basic functions
Comment on lines
+7
to
+35
| def pearsonr(x, y): | ||
| """Compute Pearson correlation. | ||
|
|
||
| :param x: 1D vector | ||
| :param y: 1D vector of the same size as x | ||
| :return: Pearson correlation coefficient | ||
| :raises TypeError: if inputs are not torch.Tensors | ||
| :raises ValueError: if inputs are not 1D, have different lengths, have length < 2 or are constant | ||
| """ | ||
| if not isinstance(x, torch.Tensor) or not isinstance(y, torch.Tensor): | ||
| raise TypeError("Function expects torch Tensors.") | ||
|
|
||
| if len(x.shape) > 1 or len(y.shape) > 1: | ||
| raise ValueError("x and y must be 1D Tensors.") | ||
|
|
||
| if len(x) != len(y): | ||
| raise ValueError("x and y must have the same length.") | ||
|
|
||
| if len(x) < 2: | ||
| raise ValueError("x and y must have length at least 2.") | ||
|
|
||
| # If an input is constant, the correlation coefficient is not defined. | ||
| if bool((x == x[0]).all()) or bool((y == y[0]).all()): | ||
| raise ValueError("Constant input, r is not defined.") | ||
|
|
||
| mx = x - torch.mean(x) | ||
| my = y - torch.mean(y) | ||
| cost = torch.sum(mx * my) / (torch.sqrt(torch.sum(mx**2)) * torch.sqrt(torch.sum(my**2))) | ||
| return torch.clamp(cost, min=-1.0, max=1.0) |
Contributor
There was a problem hiding this comment.
let's just use the scipy.stats implementation here?
Comment on lines
+58
to
+59
| mse_loss_fn = nn.MSELoss() | ||
| mse_loss = mse_loss_fn(predictions, labels) |
Contributor
There was a problem hiding this comment.
Suggested change
| mse_loss_fn = nn.MSELoss() | |
| mse_loss = mse_loss_fn(predictions, labels) | |
| mse_loss = nn.functional.mse_loss(predictions, labels) |
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.