Skip to content

Make the RMG family set configurable via settings, resolved in get_all_families - #978

Open
calvinp0 wants to merge 1 commit into
mainfrom
feature_configurable_rmg_family_set
Open

Make the RMG family set configurable via settings, resolved in get_all_families#978
calvinp0 wants to merge 1 commit into
mainfrom
feature_configurable_rmg_family_set

Conversation

@calvinp0

@calvinp0 calvinp0 commented Aug 13, 2026

Copy link
Copy Markdown
Member

Base: main. First of three — followed by #979 (deterministic ordering + authoritative pinning) and #982 (chemical gates on the family choice). Read in that order.

The bug

  • settings['rmg_family_set'] had no effect on a bare get_all_families() call, because the function's own signature default was the literal 'default'.

  • get_all_families() is the sink every family query funnels through, so threading the setting through callers leaves the sink untouched and every call site that does not forward the value keeps returning the curated set. check_family_name() on main is exactly such a bare call site (family.py:1285, verified against 44a6b112).

  • Measured on a deployed installation whose ~/.arc/settings.py asked for 'all':

    call families returned
    get_all_families('all') 100
    get_all_families('default') 55
    bare get_all_families() 55 — the setting is read and ignored
  • This is a missed call site, not a stale value: arc/imports.py merges the ~/.arc overlay at module level, so a module-level constant captures the overlaid value correctly. Only the bare call was broken.

The fix

  • One line in the sink, which already owned an in-body fallback for its own default:

    -    rmg_family_set = rmg_family_set or 'default'
    +    rmg_family_set = rmg_family_set or settings['rmg_family_set']
  • Every caller now propagates None to mean "not specified", so no call site can miss it.

  • determine_family()'s shortcut to the cached product_dicts property keys off rmg_family_set is None rather than == 'default', so an explicitly requested set is honoured even when it happens to match the configured one.

  • New setting rmg_family_set = 'default' in arc/settings/settings.py, documented in place.

Making 'all' actually reach the families

  • 'all' previously unioned only the sets named in RMG's recommended.py. Some families ship in the database without appearing in any set — Intra_RH_Add_Exocyclic and Intra_RH_Add_Endocyclic among them.
  • get_all_families() now also unions the families that exist as RMG database directories, via a new get_rmg_family_directories(). A directory counts only when it holds a groups.py template.
  • The union is de-duplicated, dropping 24 duplicate labels — every one of them the same label appearing in both default and ch_pyrolysis, so no distinct family is lost.

Acceptance case, 2-methyl-1-butene ⇌ 1,1-dimethylcyclopropane (C=C(C)CC <=> CC1(C)CC1):

rmg_family_set before after
'default' None None
'all' None Intra_RH_Add_Exocyclic

Counts come from two different RMG-database checkouts — treat them separately. The 100/55 table above is from the deployed installation. Measured locally on this branch's head against RMG-database at /home/calvin/code/RMG-database:

  • 'default' → 54 families; 'all' → 99 families.
  • 85 unique recommended-set families + 11 directory-only families = 96 RMG families, then 3 ARC families, for 99 total.
  • 45 families are in 'all' but not 'default'; 11 are in 'all' but in no recommended set at all (the pure directory scan): H2_Loss, Intra_RH_Add_Endocyclic, Intra_RH_Add_Exocyclic, Intra_R_Add_ExoTetCyclic, Li_Addition_MultipleBond, Li_NO_Ring_Opening, Li_NO_Substitution, R_Addition_CSm, SubstitutionS, Substitution_O, lone_electron_pair_bond.

This redefines 'all', and 'all' reaches a stock install

'all' is not only reachable through the setting. linear.py:1734 asks for it unconditionally whenever the configured set yields no product dicts, so an installation that never touches the setting is affected. That block is pre-existing on main and is not modified here.

Measured on the linear.py path — 1,4-cyclohexadiene ⇌ benzene + H₂, at the shipped 'default':

wider-scan product_dicts family
origin/main 0 → falls through to the heuristic path None
this PR 16 H2_Loss
  • test_wider_family_set_scan_used_by_the_linear_ts_adapter pins this.
  • Separately and pre-existing: that block restores _family / _product_dicts / _atom_map in a finally, but only after the whole weights loop, while TSGuess(family=rxn.family, ...) is written during it — so the reaction object is restored and the persisted artifacts keep the wider family. Not changed here; noted because this PR alters what that path sees.

Newly-reachable families have strictly lowest priority

  • get_rmg_family_directories() returns sorted names and get_all_families() appends them after the recommended union, so a directory-only family sits at positions 85–95 of the 96 RMG families. get_reaction_family_products iterates in order and ARC takes product_dicts[0].
  • Ordering — a directory-only family wins only when no recommended family matched, i.e. only where the alternative was family=None.
  • Isomorphism — every candidate must reproduce the products the user asserted. A directory-only family cannot introduce a transformation, only propose a mechanism for one already stated.
  • So the failure direction is "right reaction, wrong elementary path", which surfaces as NMD rejecting a good saddle — a missing rate, not a wrong one.
  • The ordering is stated in the get_all_families() docstring and pinned by test_directory_only_families_are_ordered_last and, end to end, by test_widening_keeps_the_recommended_family_when_both_match on [OH] + [O]O ⇌ OO + [O], where H_Abstraction (recommended) and Substitution_O (directory-only) each match in isolation.
  • What must be preserved is "recommended outranks directory-only", not the literal concatenation order. A flat sort across the whole list, a prepend, or a set-based dedup each break it, and each was verified to fail the ordering tests (set-dedup under five PYTHONHASHSEED values). Make reaction family determination deterministic and honour a pinned family #979 replaces the concatenation with a sort within each tier, which keeps the guarantee and additionally makes the order hash-seed-independent. Reviewers should read the two together rather than treating this PR's concatenation as final.

Why no allowlist on the directory union

  • Each of the 11 was probed by its firing conditions rather than by its exclusion: lone_electron_pair_bond needs a closed-shell singlet O (*2 = O u0 p3 c0) and returns 0 matches with triplet O; H2_Loss is one fixed 8-atom group; R_Addition_CSm needs CS itself and co-matches R_Addition_MultipleBond, which wins on ordering; the three Li_* need Li u1 p0 c0.

  • RMG's curation inverts against TS-search utility, so "RMG left it out, so it is unreliable" condemns exactly the families the benchmark needs:

    Intra_RH_Add_Endocyclic     training_rxns=0    rules=1
    Intra_RH_Add_Exocyclic      training_rxns=0    rules=1
    Intra_R_Add_ExoTetCyclic    training_rxns=0    rules=1
    Substitution_O              training_rxns=136  rules=183
    SubstitutionS               training_rxns=148  rules=1
    
  • An allowlist would be a second hand-maintained curation list drifting from ts_adapters_by_rmg_family, which ARC already maintains — and a hand-picked list would have excluded lone_electron_pair_bond, a family ARC declares support for and ships a passing test for (linear_test.py::test_interpolate_lone_electron_pair_bond). Seen that way this PR does not expose a new hazard: it makes ARC's own already-tested support reachable.

Surface families

  • 'all' contains Surface_Proton_Electron_Reduction_{Alpha, Alpha_vdW, Beta, Beta_vdW, Beta_Dissociation} on both trees — they arrive via the electrochem set, whose label contains no 'surface', so the if 'surface' not in family_set_label filter never sees them. A test pins this actual behaviour.
  • The two filters remain inconsistent (recommended path filters on set label, directory path on directory name) — pre-existing, not changed here.

Note for callers

  • A bare get_product_dicts() or get_all_families() is now sensitive to process-global settings state. That is the fix, but it is a footgun for tests and scripts: without setting rmg_family_set explicitly you are silently exercising whatever is configured, which ships as 'default'.
  • Every test added here either patches the setting or passes rmg_family_set= explicitly.

Scope and parity

  • get_all_families() at the shipped 'default' is byte-identical to main, and test_rmg_family_set_setting_ships_as_default asserts the shipped constant.
  • What is not unchanged is 'all', per the linear.py section above.
  • Full suite at parity with main — same 5 pre-existing torch_ani_test.py failures on both, no new ones, 9 tests added.
  • These tests were verified to fail against origin/main: test_wider_family_set_scan_used_by_the_linear_ts_adapter, test_widening_keeps_the_recommended_family_when_both_match, test_bare_calls_honour_the_rmg_family_set_setting, test_determine_family_reaches_a_directory_only_family.

Reuse check

  • Searched for an existing settings-resolution helper before adding anything — by behaviour (an "argument-or-fall-back-to-settings[key]" resolver), by name across def resolve* / get_setting*, and for the raw or settings[...] idiom at call sites. Nothing of the kind exists; the only resolve* helper touching settings is imports.py::resolve_overridden_dependents, which re-derives dependent paths. The fallback therefore stays inline in get_all_families(), where one already was.
  • Nothing walked the RMG database's kinetics/families (get_all_families scans ARC_FAMILIES_PATH for ARC's own families only), so get_rmg_family_directories() is new and sits beside get_rmg_recommended_family_sets().

Supersedes fix_family_set (916d9943), which carries the same feature without the sink fix.

Copilot AI lite review requested due to automatic review settings August 13, 2026 15:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@calvinp0
calvinp0 force-pushed the feature_configurable_rmg_family_set branch 2 times, most recently from a68e891 to 24becfe Compare August 13, 2026 16:13
@calvinp0 calvinp0 changed the title Make the RMG family set configurable via settings, resolved at call time Make the RMG family set configurable via settings, resolved in get_all_families Aug 13, 2026
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.20%. Comparing base (44a6b11) to head (361976b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #978      +/-   ##
==========================================
+ Coverage   64.15%   64.20%   +0.04%     
==========================================
  Files         119      119              
  Lines       39539    39557      +18     
  Branches    10261    10266       +5     
==========================================
+ Hits        25366    25396      +30     
+ Misses      11205    11189      -16     
- Partials     2968     2972       +4     
Flag Coverage Δ
functionaltests 64.20% <ø> (+0.04%) ⬆️
unittests 64.20% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…l_families

ARC hard-coded the 'default' RMG family set. It is the signature default of
get_all_families() and of every entry point above it, so there was no way to ask ARC
to consider anything else short of naming a set at each call site. That set is RMG's
recommended list, which is right for mechanism generation but excludes families ARC's
TS adapters declare support for, so a run targeting specific reactions could not reach
them at all.

A new top-level setting, rmg_family_set, now supplies that value, and it is resolved
inside get_all_families() -- the single sink every path funnels through, and which
already owned an in-body fallback for its own default. get_reaction_family_products(),
check_family_name(), and ARCReaction.get_product_dicts()/determine_family() propagate
None to mean "not specified".

Resolving it in the sink rather than in each signature is the point. Threading the
setting through the signatures instead leaves get_all_families()'s own default at the
literal 'default', so every call site that does not pass the setting on keeps returning
the curated set. Measured on a deployed installation whose ~/.arc/settings.py asked for
'all': settings['rmg_family_set'] read back as 'all', an explicit
get_all_families('all') returned 100 families, and a bare get_all_families() returned
55. On this branch's parent, check_family_name() is exactly such a bare call site.

determine_family()'s shortcut to the cached product_dicts property now keys off
rmg_family_set being None rather than equal to 'default', so an explicitly requested
set is always honoured even when it matches the configured one.

Setting rmg_family_set to 'all' is only useful if 'all' can reach the families in
question, and it could not: it unioned the family sets named in RMG's recommended.py,
and families such as Intra_RH_Add_Exocyclic and Intra_RH_Add_Endocyclic appear in none
of them despite shipping in the database. get_all_families() now also unions the
families that exist as RMG database directories, listed by the new
get_rmg_family_directories(), which counts a directory as a family only when it holds a
groups.py template. The union is de-duplicated, which also removes the 24 duplicate
labels the recommended sets alone produced.

The directories are appended after the recommended sets, so a family reachable only
through them is always positioned last and wins only where no recommended family
matched. Every candidate still has to reproduce the products the reaction asserts, so
such a family cannot introduce a transformation, only propose a mechanism for one
already stated. That ordering is a contract rather than an accident, so it is stated in
the docstring and pinned by a test: sorting the result or de-duplicating it through a
set would silently break it.

This redefines what 'all' means, and 'all' is not only reachable through the setting:
the Linear TS adapter asks for it unconditionally whenever the configured set yields no
product dicts. An installation that never touches the setting is therefore affected,
which a test over that path records -- 1,4-cyclohexadiene <=> benzene + H2 has no family
under the shipped default and resolves to H2_Loss under that retry.

get_reaction_family_products()'s docstring claimed 'all' excludes surface families
outright, and two further statements repeated it. 'all' skips family sets whose label
contains 'surface' and surface family directories, but a set with a non-surface label
can still list one, as electrochem does for the Surface_Proton_Electron_Reduction_*
families. The three statements now say what the code does, and a test pins it.

Tests cover both directions: that the shipped default leaves get_all_families()
unchanged, and that the configured set governs get_all_families(), check_family_name(),
get_product_dicts() and determine_family() when no set is named at the call site.
2-methyl-1-butene <=> 1,1-dimethylcyclopropane is covered end to end as a family
reachable only through the database directories, and OH + HO2 <=> H2O2 + O as a reaction
that matches both a recommended family and a directory-only one, where the recommended
family wins.
@calvinp0
calvinp0 force-pushed the feature_configurable_rmg_family_set branch from e4f05d8 to 361976b Compare August 15, 2026 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants