Fix position of added OXT atom

[aqemia-thomas-holder]

Problem description: The computation of the OXT atom position is wrong, it's flipped to the wrong side. Energy minimization often fixes the position, but not always. A good example is 4JSV.

Summary of changes:

• Fixed inverted sign: +2*v → -2*v
• Added pipeline test

Example before and after:

pdbfixer pdbfixer/tests/data/4JSV.pdb --output=oxtfix-4JSV.pdb --add-atoms=heavy

[peastman]

Thanks! I really appreciate the fix. I have a few suggestions for making the code clearer.

[peastman]

Why split this out into a separate method? That makes it harder to tell what's changed. From the version history it will look like this is new code, when in fact you just changed a single character.

[aqemia-thomas-holder]

Moved to a separate method for:

• Writing a unit test
• Have the option to use it in other places (currently not the case)

[peastman]

It will never be used anywhere else. Adding an OXT atom is a very specific calculation that only happens in one place.

[aqemia-thomas-holder]

I will change it to your proposed solution.

[aqemia-thomas-holder]

changed as requested

[peastman]

Where did these positions come from? How was the reference OXT position calculated?

What about using the following test instead, which makes it clearer what's really being tested?

def test_findOXTPosition():
    """Test that OXT is added at the correct position."""
    fixer = pdbfixer.PDBFixer(filename=(Path(__file__).parent / "data" / "1BHL.pdb").as_posix())

    # Record the original position of OXT, then delete it.

    originalPos = [pos for pos, atom in zip(fixer.positions, fixer.topology.atoms()) if atom.name == 'OXT']
    modeller = app.Modeller(fixer.topology, fixer.positions)
    modeller.delete([atom for atom in modeller.topology.atoms() if atom.name == 'OXT'])
    fixer.topology = modeller.topology
    fixer.positions = modeller.positions

    # Have PDBFixer add it back and make sure it is sufficiently close to the original position.

    fixer.missingResidues = {}
    fixer.findMissingAtoms()
    fixer.addMissingAtoms()
    newPos = [pos for pos, atom in zip(fixer.positions, fixer.topology.atoms()) if atom.name == 'OXT']
    assert unit.norm(newPos[0]-originalPos[0]) < 0.01*unit.nanometer

[aqemia-thomas-holder]

Where did these positions come from?

Except OXT, these positions came from https://files.rcsb.org/ligands/download/ALA.cif

How was the reference OXT position calculated?

OXT position calculated with the fixed code

What about using the following test instead

This does energy minimization, correct? I think a test without energy minimization would be more appropriate.

[peastman]

It's better to test the whole pipeline. Otherwise we aren't really testing that it works properly. The test would pass even if the code called the new function incorrectly, or never called it at all!

The important thing is that the test should fail with the current code, but pass with the fixed code. That's what gives you confidence that you're really testing what you think you are.

[aqemia-thomas-holder]

changed as requested

[peastman]

Looks good, thanks!