Fix residue naming by hannahbaumann · Pull Request #2042 · OpenFreeEnergy/openfe

hannahbaumann · 2026-06-25T13:59:26Z

Checklist

All new code is appropriately documented (user-facing code must have complete docstrings).
Added a news entry, or the changes are not user-facing.
Ran pre-commit: you can run pre-commit locally or comment on this PR with pre-commit.ci autofix.

Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).

GPU integration tests
example notebook testing
packaging tests: run this for any large feature PRs or PRs that add test data.

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

hannahbaumann · 2026-06-29T08:12:43Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

codecov · 2026-06-29T10:21:36Z

Codecov Report

❌ Patch coverage is 98.75776% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.70%. Comparing base (40d2e34) to head (c818e3e).

Files with missing lines	Patch %	Lines
src/openfe/protocols/openmm_rfe/hybridtop_units.py	96.77%	1 Missing ⚠️
...s/protocols/openmm_rfe/test_hybrid_top_protocol.py	98.86%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2042      +/-   ##
==========================================
- Coverage   95.01%   90.70%   -4.31%     
==========================================
  Files         212      213       +1     
  Lines       20572    20728     +156     
==========================================
- Hits        19546    18801     -745     
- Misses       1026     1927     +901

Flag	Coverage Δ
fast-tests	`90.70% <98.75%> (?)`
slow-tests	`?`

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.

hannahbaumann · 2026-06-29T12:10:12Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

IAlibay

This looks good thanks! - couple of things to fix but otherwise should be fine.

After this PR, will you be able to do the same for PlainMD, AFE, and SepTop?

IAlibay · 2026-07-01T13:19:37Z


        try:
-            data = rmsd.gather_rms_data(pdb_file, trj_file)
+            data = rmsd.gather_rms_data(pdb_file, trj_file, ligand_selection="resname LIG")


This will fail is someone provides their own residue names ahead of time (which is something we should encourage - i.e. power users should be rewarded not punished).

There are two potential solutions here:

(lowest cost / temporary fix) We just store the alchemical residue names in the output of the setup unit and then fetch them in the analysis unit.

(higher cost / more complicated) We pass in the alchemical components & the comp resids through all the way to the analysis unit and then extract the relevant residue names from that.

For now, I would suggest we just pass the residue names and have the residue name kwarg be optional and/or have a default of LIG, that way we don't break backwards compatibility.

We can clean up after the fact.

Note that for hybrid toplogy, it's technically possible for both endstate molecules to have different names, so we should check that it works properly.

I think a 3rd option might be best, where we do away with tracking the residue names and just keep the alchemical atom indices around. This would mean moving away from the gather_rms_data function and using the new openfe-analaysis API but we plan on doing that anyway, so this could be a good time to do it?

This would fix the case pointed out above and the fact that if a user sets the alchemical ligands and the cofactors to have the same residue name (LIG), we will still have the same issue in the analysis unit.

I wouldn't consider the above blocking though we could have this as a temp fix and move to indices when we change the analysis for this protocol?

Yes, I like the 3rd option as the long term fix, for now I went ahead with option 1, but happy to change it to a longer term fix.

Re two ligands different residue names: I tested this out and it assigns the residue name from the stateA ligand to the hybrid topology (in the HTF). We could still check for both residue names since it wouldn't hurt, but may also confuse the user?

I think a 3rd option might be best, where we do away with tracking the residue names and just keep the alchemical atom indices around.

This somewhat fits within the longer term idea of switching comp_resids to just comp_indices (where you track the indices of each component in the system). However, that's a lot bigger lift.

I don't mind just tracking the alchemical indices in the setup results - that's something we nearly already do in the AFE Protocol. But yeah, for the sake of incremental PRs = faster velocity, I would vote for the quick fix now, then the medium/long term fix after.

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

jthorton · 2026-07-02T09:35:21Z

Not blocking: Does a user have a way of setting the residue_name via a CLI input? It seems like the atom metadata fields are not preserved on writing an openff molecule to file?

hannahbaumann · 2026-07-02T09:53:30Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

IAlibay · 2026-07-02T10:26:00Z

Not blocking: Does a user have a way of setting the residue_name via a CLI input? It seems like the atom metadata fields are not preserved on writing an openff molecule to file?

I had forgotten that this was still an issue...

The problem is that residue name is a per atom PDBResidueInfo property in rdkit molecules, and a) gufe SMCs don't serialize that, and b) it doesn't get written out when you write out an SDF file. See: OpenFreeEnergy/gufe#327

hannahbaumann · 2026-07-03T11:25:39Z

After this PR, will you be able to do the same for PlainMD, AFE, and SepTop?
Yes, after having decided on the way how to address this in this PR, I can do the same for the other protocols in a follow-up PR!

jthorton

This looks great @hannahbaumann though the role of LG2 is not clear currently as the output seems to use LG1 as the name for the hybrid ligand in the output topology. Having the other name around if we write out seperate PDBs for the end states would make sense but maybe its confusing until then?

…y/openfe into fix_residue_naming

IAlibay · 2026-07-14T13:35:56Z


 logger = logging.getLogger(__name__)

+DEFAULT_LIGAND_RESNAMES = ("LG1", "LG2")


Sorry I'm going to go counter to what Josh said - I'm really not a big fan of this in such a big file. I'd prefer it being redefined than to have a hanging global variable that gets referenced effectively one time.

At worst let's just remove the default from run.

Sorry @hannahbaumann I should have been more specific I did mean just declare in execute and remove the default from run if you agree to only have it defined in one place.

Ok, removed the default from run now!

Ah, but this now results in an API break...

I think the API break is ok because we shouldn't have execution that spans multiple versions of openfe, but maybe I'm forgetting something!

hannahbaumann · 2026-07-15T08:54:55Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

IAlibay

Sorry, I kinda convinced myself to go back to LIG 😅 - happy to discuss in a call if needed.

IAlibay · 2026-07-15T12:55:56Z


        if len(selection_indices) > 0:
+            sub_top = hybrid_topology.subset(selection_indices)
+            # Renumber sequentially so same-named residues (e.g. two COF cofactors,


Do we know if resids from the original PDB make it through all the way here?

I.e. if a user has a PDB file that starts at 4 instead of 1, until you do this renumbering - would the first residue also start at 4?

The main reason I ask this is that renumbering essentially loses any input information, so in some ways it makes the user experience worse because they can't then map things back to the input.

Do the small molecules have a different chain ID from the protein residues? If so, would assigning the residue number at the same time we assign residue names to the offmol be suitable?

I checked it and the small molecules are on a separate chain from the protein, so I'll switch to your suggestion and assign the residue number at the same time as the residue name!

IAlibay · 2026-07-15T12:58:36Z

+                name = base if base not in used else _unique(used)
+                used.add(name)
+            else:
+                name = "COF"


[nit] this technically allows a case where the ligand(s) are named COF and the cofactors also get called COF.

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

hannahbaumann · 2026-07-16T11:42:45Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

IAlibay

Looking great!
Couple of small things - mostly the test and the silent faillure with the get.

IAlibay · 2026-07-16T12:31:59Z

+            name = _get_offmol_resname(small_mols[comp])
+            assert name is not None
+            names.add(name)
+        alchem_resnames = sorted(names)


[nit] not something to fix now, but just an FYI that long term we might need to know what name belongs to what end state.

IAlibay · 2026-07-16T12:49:29Z

            pdb_file=pdb_file,
            trajectory=trajectory,
            checkpoint=checkpoint,
+            ligand_resnames=setup_results.outputs.get("alchemical_resnames"),


This will silently get None if the entry isn't there. it will eventually just make structural analysis fail completely silentlly (because it's alll wrapped in a try/except).

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

…y/openfe into fix_residue_naming

IAlibay

lgtm thanks!

github-actions · 2026-07-24T13:58:06Z

🚨 API breaking changes detected! 🚨

View workflow run

Griffe output

$ griffe check "openfe" -s src --no-inspection --no-color --verbose -a origin/main
src/openfe/protocols/openmm_rfe/hybridtop_units.py:1621: HybridTopologyMultiStateAnalysisUnit.run(ligand_resnames):
Parameter was added as required


$ griffe check "openfecli" -s src --no-inspection --no-color --verbose -a origin/main

Initial change of lig and cofactor resname

e7882f0

jameseastwood assigned hannahbaumann Jun 25, 2026

Merge branch 'main' into fix_residue_naming

cb4ece2

[pre-commit.ci] auto fixes from pre-commit.com hooks

a32cf02

for more information, see https://pre-commit.ci

hannahbaumann mentioned this pull request Jun 29, 2026

Fix ligand selection when cofactors share resname UNK OpenFreeEnergy/openfe_analysis#119

Open

Fix tests

369f9d5

hannahbaumann added 2 commits June 29, 2026 13:48

Add news entry

69a6a08

Add more tests

2609bc4

[pre-commit.ci] auto fixes from pre-commit.com hooks

0844937

for more information, see https://pre-commit.ci

hannahbaumann changed the title ~~[WIP] Fix residue naming~~ Fix residue naming Jun 29, 2026

hannahbaumann requested review from IAlibay and jthorton June 29, 2026 12:14

hannahbaumann linked an issue Jun 29, 2026 that may be closed by this pull request

Change ligand and cofactor residue names in the topology #2050

Open

hannahbaumann and others added 2 commits June 29, 2026 14:58

Merge branch 'main' into fix_residue_naming

91fde04

Merge branch 'main' into fix_residue_naming

d7a36cd

IAlibay requested changes Jul 1, 2026

View reviewed changes

Update src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py

11b9c2c

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Address review comments

12b9840

[pre-commit.ci] auto fixes from pre-commit.com hooks

c68b88e

for more information, see https://pre-commit.ci

jameseastwood assigned IAlibay and hannahbaumann and unassigned hannahbaumann Jul 2, 2026

jthorton reviewed Jul 14, 2026

View reviewed changes

Comment thread src/openfe/protocols/openmm_rfe/hybridtop_units.py Outdated

jthorton reviewed Jul 14, 2026

View reviewed changes

hannahbaumann added 5 commits July 14, 2026 12:35

address review comments

cea0a89

Merge branch 'fix_residue_naming' of https://github.com/OpenFreeEnerg…

aa8fba2

…y/openfe into fix_residue_naming

Fix mypy

c63caaa

update comments

4729087

update comments

9e4b9d2

IAlibay reviewed Jul 14, 2026

View reviewed changes

Address review comments

aaa22c5

[pre-commit.ci] auto fixes from pre-commit.com hooks

6b31ed5

for more information, see https://pre-commit.ci

hannahbaumann requested review from IAlibay and jthorton July 15, 2026 11:58

IAlibay requested changes Jul 15, 2026

View reviewed changes

hannahbaumann and others added 4 commits July 15, 2026 17:01

Update news/fix_unk_resnames.rst

1bc9366

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Address review comments

bc2bb0b

Address review comments

73d9c93

small fix

e15d56d

[pre-commit.ci] auto fixes from pre-commit.com hooks

c818e3e

for more information, see https://pre-commit.ci

hannahbaumann requested a review from IAlibay July 16, 2026 11:50

IAlibay requested changes Jul 16, 2026

View reviewed changes

hannahbaumann commented Jul 16, 2026

View reviewed changes

Comment thread news/fix_unk_resnames.rst Outdated

hannahbaumann and others added 4 commits July 16, 2026 14:52

Apply suggestion from @hannahbaumann

7277c00

Apply suggestion from @IAlibay

7057e15

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

fix alchem_resname grab

f0ac4f8

Merge branch 'fix_residue_naming' of https://github.com/OpenFreeEnerg…

8b07a3d

…y/openfe into fix_residue_naming

IAlibay approved these changes Jul 16, 2026

View reviewed changes

Merge branch 'main' into fix_residue_naming

4c8a2ea


		logger = logging.getLogger(__name__)

		DEFAULT_LIGAND_RESNAMES = ("LG1", "LG2")

Uh oh!

Conversation

hannahbaumann commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developers certificate of origin

Uh oh!

hannahbaumann commented Jun 29, 2026

Uh oh!

codecov Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hannahbaumann commented Jun 29, 2026

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton commented Jul 2, 2026

Uh oh!

hannahbaumann commented Jul 2, 2026

Uh oh!

IAlibay commented Jul 2, 2026

Uh oh!

hannahbaumann commented Jul 3, 2026

Uh oh!

Uh oh!

jthorton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannahbaumann commented Jul 15, 2026

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hannahbaumann commented Jul 16, 2026

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hannahbaumann commented Jun 25, 2026 •

edited

Loading

codecov Bot commented Jun 29, 2026 •

edited

Loading