Repository navigation
Radius filter for RetroRules v3 rule sets, plus a performance fix in the expansion loop - #37
Open
duartebred wants to merge 20 commits into
Open
duartebred wants to merge 20 commits into
duartebred wants to merge 20 commits into
Conversation
…ring - Introduce `diameter` parameter to control reaction rule environment size. - Reorder constructor parameters for clarity. - Update docstring to explain new parameter.
…etro (trabalho de 2025)
O notebook constroi um ficheiro de dados a partir do RetroRules 2019; nao altera a ferramenta e por isso nao e contribuicao para o BioCatalyzer. Foi movido para notebooks/legacy/ no projeto da dissertacao a 13/08/2026 e nao deve entrar no Pull Request.
O --diameter foi escrito em 2025 para a nomenclatura do RetroRules 2019, onde os templates eram identificados por diametro. O RetroRules v3.0.0 gera templates por raio 0-10 e nao publica diametro, pelo que manter os dois nomes exporia dois parametros para o mesmo conceito. A implementacao tinha alem disso dois defeitos por corrigir: loaders.py exigia uma coluna Diameter que o ficheiro de regras distribuido com a ferramenta nao tem, fazendo falhar qualquer corrida com as regras default; e matcher.py:211 continuava a chamar load_reaction_rules com a assinatura antiga. Os quatro ficheiros sao restaurados ao estado upstream. O trabalho de 2025 fica preservado no branch snapshot-2025.
…the dataframe _react_single resolved the reactants, the internal ID and the EC numbers of the rule being applied with three boolean masks over the reaction rules dataframe, plus one over the compounds dataframe, i.e. four full table scans for every (compound, rule) pair. The cost grows linearly with the size of the rule set and quickly dominates the chemistry itself: with 58 597 rules the lookups take ~19.9 ms per pair against ~0.3 ms for the RDKit reaction, so over 98% of the wall-clock time is spent scanning. The mappings are now built once, by _index_reaction_rules, and _react_single reads them directly. SMARTS strings are unique within a BioCatalyzer rule set, so the dictionary returns exactly what the mask returned and the output is unchanged. The index duplicates state that already lives in the dataframes, so it is rebuilt at every point where either dataframe is reassigned: the constructor and the compounds, compounds_path, neutralize, reaction_rules and organisms_path setters. Missing any of these would leave the index stale and _react_single would resolve the wrong rule, or raise a KeyError, without any warning. Verified on 5 compounds x 3556 rules: identical 76 products, 34.2 s -> 5.5 s.
…lare Radii
RetroRules v3.0.0 publishes one template per radius (0-10) and states the radii a
template was modelled at in a Radii field. Radius controls how much environment a
SMARTS pattern encodes and therefore how promiscuous it is, so it is the natural
lever against the combinatorial explosion of a rule-based expansion.
load_reaction_rules gains a `radius` argument accepting an integer (6), a
;-separated list ('4;6;8') or an inclusive range ('4:8'). A rule is kept when any
of the requested radii appears in its Radii field: the same membership test the
Organisms filter already uses, so the two filters compose naturally. Membership
rather than equality is required because one SMARTS can be generated at several
radii, and a template first seen at radius 1 and still valid at radius 6 belongs
to both sets.
The filter is opt-in and column-driven. It defaults to 'ALL' and is skipped, with
a warning, on rule sets that carry no Radii column, so the bundled rule files and
any user-supplied file behave exactly as before.
BioReactor stores the setting and reapplies it wherever the rules are reloaded,
including the organisms_path setter, which would otherwise silently drop the
radius filter when the organism set changed.
Surfaces the reaction rule radius filter added to Loaders.load_reaction_rules. Defaults to 'ALL', which reproduces the previous behaviour of both CLIs.
Nine tests over a fixture whose radius membership is known by construction: rule i is modelled at every radius that is a multiple of i+1, so the expected selection for each of the eleven radii is computed independently of the implementation. Covers each radius from 0 to 10 individually, the integer, list and range spellings, composition with the organism filter, and _parse_radius. Two tests document design decisions that are otherwise invisible: - test_subsets_are_not_disjoint asserts that the radius subsets sum to more than the rule count, which is why membership rather than equality with a single modelled radius is the correct test. - test_rule_set_without_radii_column_is_untouched asserts that rule files carrying no Radii column, including those distributed with the tool, are returned unfiltered with a warning.
Changes the default behaviour of the tool: rule sets that declare a Radii column are now filtered to radius 6 unless the caller says otherwise. Radius 6 was adopted as the operating point . Callers opt out with radius='ALL', or select any other radius or range. The warning emitted when a rule set declares no Radii column had to be rewritten. It read 'a radius filter was requested', which was true only while the default was 'ALL'; with a filtering default it would fire on every run against the rule files shipped with the tool, complaining about a filter the user never asked for. It now states the radius in effect, that no rules were discarded, and how to silence it. Tests: test_default_applies_no_filter replaced by test_default_is_radius_6, which asserts that the implicit and explicit calls agree, plus a new test_all_disables_the_filter covering the opt-out. Suite: 55 passed, 12 failed -- the same 12 pre-existing failures, caused by unquoted path concatenation in the CLI test harness. Coverage 82% -> 84%.
The default reaction rule set distributed with the tool -- what is used when no --reaction_rules is given -- is replaced with the converted RetroRules v3.0.0 rule set: 337 901 rules, up from 22 949, still bzip2-compressed (16.5 MB, comparable in size to the file it replaces). Two columns present in the old file, RuleID and ReactionRuleSource, are absent from the RetroRules-derived data. Checked against the codebase and the test suite: neither is read anywhere. ReactionRuleSource is nonetheless populated with 'RetroRules v3.0.0' on every row, to preserve the provenance information the old file carried rather than drop it silently. Three assertions in test_bioreactor.py compared the loaded rule table against the exact shape of the old file (e.g. (7102, 7)) and failed once the file changed size and gained columns (13 vs 7) -- expected, since the rule count is a property of the data file, not of the code under test. Replaced with checks for what the code actually depends on: that rules were loaded, and that the SMARTS and Organisms columns exist. The one assertion on an output shape -- (3220, 7) -- had its row count relaxed for the same reason, while the column count (7) is kept exact, since that one is fixed by the code and would be worth catching if it changed. Full suite: 55 passed, 12 failed -- the same 12 pre-existing failures caused by unquoted path concatenation in the CLI test harness, unrelated to this change. Approved by J. Correia to replace the bundled file rather than only accept the rule set via --reaction_rules.
duartebred
force-pushed
the
integrate-reaction-rules
branch
from
September 25, 2026 15:47
c72129a to
3655c70
Compare
Collaborator
|
@duartebred I just pushed some small fixes:
After this, you can merge the PR and delete the branch. Note: The description of the PR is a little outdated with a lot of old info. |
jcorreia11
approved these changes
Oct 12, 2026
…ide the output folder
jcorreia11
requested changes
Oct 12, 2026
jcorreia11
left a comment
jcorreia11
left a comment
Collaborator
There was a problem hiding this comment.
After closer review I found some issues that i need to check up with you:
-
Compared with the old data we dropped data from pickaxe (this way we lose, for instance, spontaneous reactions). I am ok with adding those latter but we should at least open an issue for that.
-
Is the organism annotation correct and complete?
There are a lot of reaction rules with EC numbers that do not have associated organisms... It looks like you only added the organisms from the gut microbiome. You should annotate the reaction rules using all organisms available and then on you experiments filer for the ones you want... For instance, there are 0 rules annotated with human enzymes (hsa).
Also, it would be nice if you describe in the PR description how did you do the organism annotation.
- With the increase of reaction rules we could face some OOM problems... maybe we should optimize this. I am also ok with checking this later but we should open an issue for that.
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.
This PR collects the work done to make BioCatalyzer usable with the RetroRules
v3.0.0 template collection, as part of my MSc dissertation. Four code commits,
each independent and reviewable on its own.
refactor: remove the--diameterparameter in favour of--radiusI added
--diameterin 2025 for the 2019 RetroRules nomenclature. RetroRulesv3.0.0 annotates templates by radius (0–10) rather than diameter, so keeping both
names would expose two parameters for one concept.
The implementation also had two unresolved defects:
loaders.pyrequired aDiametercolumn that the bundled rule file does not have, which made any runwith the default rules fail; and
matcher.py:211still calledload_reaction_ruleswith the old signature. Removing it restores the tool'soriginal behaviour with its own rule set.
perf: index reaction rules by SMARTS instead of scanning the dataframe_react_singleresolved the reactants, the internal ID and the EC numbers withthree boolean masks over the rules dataframe, plus one over the compounds
dataframe — four full table scans per (compound, rule) pair. With 58 597 rules
these lookups take ~19.9 ms per pair against ~0.3 ms for the RDKit reaction, so
over 98% of the runtime was table traversal.
The mappings are now built once. SMARTS are unique within a rule set, so the
dictionary returns exactly what the mask returned. The index is rebuilt at all
six points where either dataframe is reassigned — the constructor and the
compounds,compounds_path,neutralize,reaction_rulesandorganisms_pathsetters — since a stale index would resolve the wrong rulesilently.
Verified on 5 compounds × 3 556 rules: identical 76 products, 34.2 s → 5.5 s.
feat: optional radius filter, and--radiusin both CLIsAccepts an integer (
6), a;-separated list (4;6;8) or an inclusive range(
4:8). A rule is kept when any requested radius appears in itsRadiifield —membership rather than equality, because one SMARTS can be generated at several
radii, so a template first seen at radius 1 and still valid at radius 6 belongs
to both sets. This is the same membership test the
Organismsfilter alreadyuses.
The filter is opt-in and column-driven: it defaults to
ALLand is skipped, witha warning, on rule sets without a
Radiicolumn, so the bundled files behaveexactly as before.
Tests
From 37 passed / 20 failed to 45 passed / 12 failed; coverage 58% → 82%. The
eight tests that now pass were the ones broken by the incomplete
--diameterwork. The remaining twelve are all in
tests/unit_tests/clis/and fail becausethe test harness builds commands by unquoted string concatenation, which breaks
on any absolute path containing spaces — unrelated to this PR.
Note on the older commits
The branch also carries commits from Feb/May 2025 and Aug 2026 that were never
merged: two
requirements.txtfixes, the removal of a notebook that builds adata file and is not a contribution to the tool, and the 2025
--diametersnapshot that the first commit above reverts. Happy to rebase them out if you
prefer a narrower diff.
Known upstream issues found along the way, not addressed here
cli.py:115callslogging.basicConfigwith a path inside the output folderbefore
BioReactorcreates it, so a run fails if the folder does not exist.react()materialises the full compound × rule Cartesian product as a list,and then a second list of the same pairs — 11.2M pairs for a 337 901-rule file.
Happy to open separate PRs for either.