Repository navigation
Conversation
intermediate/chembl_molecule gets ten columns for every molecule, read from the same ChEMBL dump: mwFreebase, alogp, aromaticRings, heavyAtoms, qedWeighted, numRo5Violations (compound_properties), structuralAlerts (every alert of every set), structureType, naturalProduct and moleculeClass (the ChEMBL molecule type, with Proteins of 1500 Da or less labelled Peptide). The existing columns are built as before, and drug_molecule selects only its own schema, so output/drug_molecule does not change. The joins validate 1:1 keys, so a duplicated molregno fails the step instead of duplicating molecules. Checked on platform-2610-1 (ChEMBL 37), running the new step on that run's intermediate: 2,921,148 rows and ids identical, the 11 existing columns identical; drug_molecule rerun on it is identical (19,179 rows). Parquet 986 -> 1,030 MB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d0choa
left a comment
There was a problem hiding this comment.
Thanks for this, the PR description and the real-data check are really helpful. I'm requesting changes for the inline comments, but before going further I'd like to understand the scope better.
Are we planning to use all the fields that come in here? Do you envision showing them on the drug profile pages? And are all of them relevant to the tractability assessment, or are some just nice to have? Each column adds to the intermediate and is something we have to maintain and keep correct across ChEMBL releases. I'd rather add only the ones with a clear consumer, and bring in the others when something needs them.
| pl.col('chembl_id').alias('id'), | ||
| 'molecule_type', | ||
| pl.col('structure_type').alias('structureType'), | ||
| (pl.col('natural_product') == 1).alias('naturalProduct'), |
There was a problem hiding this comment.
== 1 turns ChEMBL's default into a definite False. As far as I know ChEMBL only curates natural_product for drugs, so for the ~2.9M other molecules a 0 means "not curated" rather than "not a natural product". A consumer would read False as a confident negative when the true answer is unknown. Any other value ChEMBL might use would also silently become False.
Together with the imatinib case you mention in the notes, I'm not sure how reliable this flag is. Are we planning to use naturalProduct for anything? If not, I'd leave it out. If we are, it should probably be null outside the curated set.
| molecule_type = pl.col('molecule_type') | ||
| return ( | ||
| pl | ||
| .when((molecule_type == 'Protein') & (pl.col('mwFreebase') <= PEPTIDE_MAX_MW)) |
There was a problem hiding this comment.
Can you explain the rationale for the Peptide class? Why do we need it, and what will consume it? It isn't a ChEMBL class, and the 1,500 Da cut-off isn't documented anywhere. Where does that number come from?
There are also a couple of consistency issues with the rule as written:
- It uses each molecule's own
mw_freebase. A salt or child form with nocompound_propertiesrow staysProteinwhile its parent becomesPeptide, so one drug family can carry mixed classes. - The ~600 Proteins with no MW never get reclassified, even if they are short sequences.
If we keep it, deriving the class from the parent molecule would avoid the mixed families.
| .select('id', *molecule_properties_schema.keys()) | ||
| .cast(molecule_properties_schema) | ||
| ) | ||
| return molecules.join(annotations, on='id', how='left', validate='1:1', maintain_order='left') |
There was a problem hiding this comment.
Suggested cleanup: process_molecules drops molregno, so this joins molecule_dictionary (2.9M rows) in a second time just to map chembl_id back to molregno, then joins back onto the molecules on a string id. If _molecule_preprocess kept molregno and dropped it only at the final select, properties and alerts could join straight onto the molecules. That removes the second molecule_dictionary pass, the id-keyed join and its separate validate='1:1' failure mode.
| pl | ||
| .when((molecule_type == 'Protein') & (pl.col('mwFreebase') <= PEPTIDE_MAX_MW)) | ||
| .then(pl.lit('Peptide')) | ||
| .otherwise(molecule_type.fill_null('Unknown')) |
There was a problem hiding this comment.
This repeats the molecule_type.fill_null('Unknown') rule that already produces drugType in _molecule_preprocess. If the two copies drift, moleculeClass will no longer match drugType outside the Peptide override, which is the equivalence the PR relies on. Could moleculeClass be derived from drugType instead?
| @@ -21,6 +21,22 @@ | |||
| }) | |||
| """The chembl_molecule intermediate, as the drug_molecule step consumes it.""" | |||
There was a problem hiding this comment.
This docstring still says molecule_schema is the chembl_molecule intermediate, but the step now writes molecule_schema plus molecule_properties_schema, so no single schema describes the output. Could we either define the intermediate's full schema (e.g. {**molecule_schema, **molecule_properties_schema}) or update this docstring?
| Returns: | ||
| The molecules with the ``molecule_properties_schema`` columns appended. | ||
| """ | ||
| # postgres `numeric` columns arrive as decimals |
There was a problem hiding this comment.
This comment sits above a select that only renames columns. The Decimal → Float64 conversion happens much later, in .cast(molecule_properties_schema), so _molecule_class compares a still-Decimal mwFreebase against PEPTIDE_MAX_MW. Casting the decimal columns in this select would make the comment true and mean the 1,500 Da comparison runs on the final type.
| rows_by_id(_process(self._tables(p)))['CHEMBL0001']['childChemblIds'] | ||
| for p in self._permutations() | ||
| ] | ||
| results = [rows_by_id(_process(self._tables(p)))['CHEMBL0001']['childChemblIds'] for p in self._permutations()] |
There was a problem hiding this comment.
This hunk and the compound_structures schema one above (line 407) are formatting-only changes to existing tests. Could you drop them from this PR? They make the diff and the blame bigger without changing behaviour.
What
intermediate/chembl_moleculegets ten columns for every molecule, read from the ChEMBL dump the step already restores. They are for the tractability work; no published output reads them yet.mwFreebase,alogp,qedWeightedcompound_propertiesaromaticRings,heavyAtoms,numRo5Violationscompound_propertiesstructuralAlerts{alertId, alertName, setName}compound_structural_alerts,structural_alerts,structural_alert_sets(all five sets)structureTypemolecule_dictionarynaturalProductmolecule_dictionary(natural_product = 1)moleculeClassmolecule_type; aProteinof 1,500 Da or less (PEPTIDE_MAX_MW) isPeptide, and a missing type isUnknown, as indrugTypeHow
TABLESreads two moremolecule_dictionarycolumns and four more tables from the same dump. There are no new sources, soconfig.yamland the DAG don't change.add_compound_propertiesruns afterprocess_molecules, which is unchanged. It left-joins on the ChEMBL id, so every molecule is kept. Molecules with no properties row get nulls, and molecules with no alerts get an empty list.validate='1:1', so a duplicatedmolregnoor id fails the step instead of duplicating molecules.molecule_properties_schemainpts/schemas/drug_molecule.py. The existing schemas don't change.Effect on existing outputs
None:
drug_moleculeselectsdrug_molecule_schema.chemical_probesreadsid.pharmacogeneticsreadsidandname.clinical_reportbuilds its drug label lookup fromid,name,synonyms,tradeNamesandcrossReferences.Checked on real data (
do/platform-2610-1, ChEMBL 37)I ran the new step on that run's
intermediate/chembl_molecule, with the five tables restored from itsinput/chembl/chembl.tar.gz.drug_moleculererun on the result: identical to the run'soutput/drug_molecule(19,179 rows, same schema, all columns).mwFreebase0.67%; the other properties 3.31%. ChEMBL doesn't compute them for most peptides.moleculeClass: 15,533 Peptides (MW 180–1,499.83) and 7,266 Proteins, 600 of them with no MW. Every other class matchesdrugType.Notes
natural_productflag is set on some synthetic drugs (for example imatinib, CHEMBL941), sonaturalProductshould be read with care.Tests
tests/test_chembl_molecule.py, including the 1,500 Da edge, a Protein with no MW, a null type and duplicated keys.uv run pytest -q: 2,816 passed, 3 skipped.ruffandtyare clean.🤖 Generated with Claude Code