Add support for None bond anchors

[proteneer]

This PR adds support for handling dummy groups that are completely unanchored (i.e. devoid of bond anchors). The previous behavior was to omit the unanchored dummy groups entirely from the output of generate_dummy_group_assignments.

[maxentile]

Is there some weaker property than num_connected_components == 1 we should still assert here?

I don't know what the correct behavior is, but maybe something like "for every connected_component in the input topology, assert that num_connected_components(attach_dumy_atoms(connected_component)) == 1"

[proteneer]

I thought about this a bit... and I'm still not sure how to implement something like attach_dumy_atoms at the "generic" level if we need to support AHFE. I think having something like this being re-implemented later on specifically for RBFE/RHFE code-paths would make a lot of sense

[mcwitt]

Core is not empty here?

[proteneer]

fixed - meant to say:

# Test that providing an empty core and a None bond anchor results in a None angle anchor
anchored_dummy_group_assignments = generate_anchored_dummy_group_assignments(
    {None: {0, 1, 2, 3, 4}}, g_a, g_b, [], []
)

[mcwitt]

(sorry, accidentally hit the "Close with comment" button and reopened)

[mcwitt]

Should the type of the dummy_groups argument to this function be updated to dict[Optional[int], frozenset[int]]?

Will the restriction of this representation to only one unanchored dummy group be an issue in practice?

[proteneer]

Will the restriction of this representation to only one unanchored dummy group be an issue in practice?

1. Multiple disconnected dummy groups (and their atoms) will all be assigned to the "None" dummy group.

def test_multiple_none_dummy_groups():
    # Test that if we break a core-core bond, we only have one valid
    # choice of the angle anchor
    #
    # mol_a: H-O-H.H-O-H
    # mol_b: H-O-H.H-O-H
    g_a = convert_to_nx(Chem.AddHs(Chem.MolFromSmiles("O.O")))
    core_a = []

    dgas = list(generate_dummy_group_assignments(g_a, core_a))
    expected_dga = {None: {0,1,2,3,4,5}}

    assert equivalent_assignment(dgas, [expected_dga])

2. As a result, their dummy angle anchors will also be assigned to the "None" angle anchor:

def test_multiple_none_dummy_groups():
    # Test that if we break a core-core bond, we only have one valid
    # choice of the angle anchor
    #
    # mol_a: H-O-H.H-O-H
    # mol_b: H-O-H.H-O-H
    g_a = convert_to_nx(Chem.AddHs(Chem.MolFromSmiles("O.O")))
    g_b = convert_to_nx(Chem.AddHs(Chem.MolFromSmiles("O.O")))
    core_a = []
    core_b = []

    # Test that providing an empty core and a None bond anchor results in a None angle anchor
    anchored_dummy_group_assignments = generate_anchored_dummy_group_assignments(
        {None: {0, 1, 2, 3, 4, 5}}, g_a, g_b, core_a, core_b
    )

    anchored_dummy_group_assignments = list(anchored_dummy_group_assignments)

    assert anchored_dummy_group_assignments == [{None: (None, frozenset({0, 1, 2, 3, 4, 5}))}]

[proteneer]

Added a test in ade39b7

[mcwitt]

Multiple disconnected dummy groups (and their atoms) will all be assigned to the "None" dummy group.

Got it. I wanted to be sure that the apparent behavior of unioning into a single group dummy groups that are disconnected (both from the core and each other) was intentional (versus using a different representation that preserves information about them being disconnected; I don't think I'm totally clear on the use case for this, so this may not be a useful comment)

[mcwitt]

Should the type of the dummy_groups argument to this function be updated to dict[Optional[int], frozenset[int]]?

IIUC, both the type of the dummy_groups argument and the return type of generate_anchored_dummy_group_assignments should be updated to indicate that the bond anchor might be None.

[proteneer]

Yep, updated in 727b151

[mcwitt]

Should this be updated to

Suggested change

	) -> Iterator[dict[int, frozenset[int]]]:
	) -> Iterator[dict[Optional[int], frozenset[int]]]:

?

Is there an issue if there are multiple unanchored dummy groups? The current behavior seems to be to keep only the last one(?)

[proteneer]

Hm I haven't actually thought about how multiple unanchored dummy groups would work in practice - e.g. if we're decoupling 2 separate molecules from the environment, both of which have no anchors?

[proteneer]

(See above comment for intended behavior)

[mcwitt]

I think the return type should still be updated, though? (To reflect that the anchor can be None; I'm not sure why mypy doesn't complain about this)

[proteneer]

Updated type signatures in 727b151

[mcwitt]

(Looks like the type of find_dummy_groups_and_anchors also needs to be updated)

[proteneer]

Thanks - looks like I may have a mypy version mismatch issue locally again (since it passed the local git precommit).

[mcwitt]

Might be because the pre-commit git hook only runs on files in the diff by default, while in CI we run with the --all-files flag

[mcwitt]

nit/optional: noting that this change to an optional assertion reduces the power of static checking to catch errors somewhat, because now the bond anchor is optional in the return type even if we pass assert_single_connected_component=True (and thus downstream code not intended to handle empty cores or disconnected graphs must assert bond_anchor is not None).

One way around this could be to instead define a variant generate_dummy_group_assignments_connected that calls this function with assert_single_connected_component=True and returns the old type (i.e. asserting in the types that the dummy anchor is not null). Another option could be to use typing.overload.

(Non-blocking, OK to clean this up later.)

[proteneer]

This is temporary - it will be non-optional (i.e. the assertion will default to False after the refactoring is done), and the argument will be removed.