HybridTopologyFactory missing tests

[jthorton]

Describe the bug

Some functions in the HybridTopologyFactory, such as the old_positions and new_positions are currently broken and I assume not tested directly in this package. As I understand it a lot of the testing is done in feflow, and as such, these functions seem to have been fixed. I think we need to at least vendor the factory updates from feflow and decide on how to handle tests. We should either copy over the tests and adapt them to meet our needs (remove the OE requirement) or implement our own set of tests to ensure that updates to the factory don't break current behaviour.

I would like to get some thoughts here on what we should do generally about the HybridTopologyFactory going forward.

[IAlibay]

I think we need to at least vendor the factory updates from feflow and decide on how to handle tests.

I'm ok with vendoring the updates for now - although would need to see if they do what we need them to do. I remember some of the changes being made around those methods in a way that might not have led to the behaviour needed (I think it's more about the mdtraj topology creation - I completely rewrote that originally, and I believe there was a difference of opinion on that).

I.e. what I'm saying is that vendoring should be spot changes rather than the whole file - that way we can review them & double check.

We should either copy over the tests and adapt them to meet our needs (remove the OE requirement) or implement our own set of tests to ensure that updates to the factory don't break current behaviour.

So I have some conflicting thoughts here, mostly in terms of FTE vs my own science/code wants.

I'll try to separate them.

The lowest cost perspective

If the code works in FEFlow, and the tests are doing the right thing there, then the hybrid topology does the right thing here.
I.e. For now, we aren't doing anything fancy that means that behaviour would break here.

From an FTE / management perspective

We have to be careful about the cost of doing this. Personally I would want us to do the minimal amount of work necessarry.
Ideally, this would mean removing the OE/Perses needs in FEFlow directly. Then at that point, there's no cost to copying tests around, they should work everywhere.

The medium / long term perspective

Removing OE / Perses from the hybridtop tests is a need, it's something we (talking for both the openfe and feflow repositories) have on our TODO list, but never got around to doing, hence why we vendor and hope for the best (not a strategy I like).

Personally, I am of the opinion that dealing with this is a need for the dummy atom fixes, because we will be changing that code and we need to make sure the hybridtop still behaves how we want it to.

The science perspective

It has and continues to strongly bother me that we don't regularly test these, especially without OEChem & with direct OpenFE tooling. With my science lead hat on, I would like to see that fixed.

[IAlibay]

Maybe a summary of all my thoughts here are "you should port the functionality & tests that you need to be able to achieve the dummy atom project objectives".

[mikemhenry]

Can confirm these are untested: https://app.codecov.io/gh/OpenFreeEnergy/openfe/blob/main/openfe%2Fprotocols%2Fopenmm_rfe%2F_rfe_utils%2Frelative.py#L2406

[ijpulidos]

What is motivating having two implementations of the HTF in feflow and openfe? I thought it was just a OE issue, but if there are plans on maintaining/developing an implementation of it in openfe, then we should probably use it as well in feflow. Regardless where it ends up landing, I think we should try to have only a single source of truth for building the hybrid topologies.

[ijpulidos]

Happy to collaborate in trying to port the tests on the HTF in feflow to not use OE and rewrite them using RDKit or similar.