proteneer / timemachine

Differentiate all the things!
Other
138 stars 17 forks source link

[wip] Don't return cores that will trigger a SingleTopology constructor error #1332

Closed maxentile closed 1 month ago

maxentile commented 1 month ago

Adds a post-filter to get_cores(mol_a, mol_b) to avoid returning any core such that SingleTopology(mol_a, mol_b, core) will raise a ChiralConversionError here https://github.com/proteneer/timemachine/blob/c63bc931cc57e89f9e18d4174bf212ab4a92209b/timemachine/fe/single_topology.py#L977

Note: This can result in get_cores returning an empty list. Perhaps in the future this filter can be applied in a different way (so we are more likely to return at least one valid core, even if smaller), or additional changes can be made to avoid the need for the assertion.

maxentile commented 1 month ago

Hmm, https://github.com/proteneer/timemachine/blob/02d5366ffdeb7ee3f5c49e4f69e5dc10b7942c3a/tests/test_single_topology.py#L858-L884 is failing, which indicates I have introduced some inconsistency between

new function has_chiral_atom_inconsistencies

https://github.com/proteneer/timemachine/blob/1b75a5991b48cac448e9a0ef194f7fdfd9cf4e9e/timemachine/fe/chiral_utils.py#L263-L287

and existing functions assert_chiral_consistency_and_validity / check_chiral_validity

https://github.com/proteneer/timemachine/blob/1b75a5991b48cac448e9a0ef194f7fdfd9cf4e9e/timemachine/fe/single_topology.py#L986-L1024

...

should modify to reuse assert_chiral_consistency_and_validity...

maxentile commented 1 month ago

Closing for now -- looks involved. The chiral filter that's currently applied during the MCS search differs from the chiral assertions that are applied in the SingleTopology constructor. I think the SingleTopology constructor is more likely to be correct.

I can't modify the check in atom_mapping.py to import the check from single_topology.py without introducing circular imports. https://github.com/proteneer/timemachine/pull/1333 is soon going to make it feasible to import and use the check from single_topology.py , so we can use the solution in https://github.com/proteneer/timemachine/pull/1315