openforcefield / openff-qcsubmit

Automated tools for submitting molecules to QCFractal
https://openff-qcsubmit.readthedocs.io/en/latest/index.html
MIT License
26 stars 4 forks source link

Add `aromatic_matching=False` to isometry check #260

Closed mattwthompson closed 4 months ago

mattwthompson commented 5 months ago

Description

Taking @lilyminium's suggestion in #255 and just seeing what happens in tests.

Not squatting on this at all; if somebody wants to move this forward please feel free to use this branch/PR.

Todos

Notable points that this PR has either accomplished or will accomplish.

Status

codecov[bot] commented 5 months ago

Codecov Report

Merging #260 (d48e333) into main (60f4210) will not change coverage. Report is 1 commits behind head on main. The diff coverage is n/a.

:exclamation: Current head d48e333 differs from pull request most recent head 77dd11b. Consider uploading reports for the commit 77dd11b to get more accurate results

Additional details and impacted files
j-wags commented 5 months ago

I looked through the code to make sure that this wouldn't cause any unanticipated surprises and it looks good at a first pass. I'd approve this PR if a test is added.

mattwthompson commented 4 months ago

Just a note to myself: adding a test like test_componentresult_deduplication_iso (or some others in the same file) with an appropriate molecule should cover this change. IIUC the key behavior is that two molecules (or two representations of, or two tautomers of, depending on your view)

The molecule in the linked issue appears to trigger these conditions via having a proton hop around within an aromatic ring

mattwthompson commented 4 months ago

Test added! If I revert the aromatic_matching=False change this test fails; with the change, the test passes.

jthorton commented 4 months ago

Great job guys!