Add aromatic_matching=False to isometry check

[mattwthompson]

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.

• Add aromatic_matching=False to isometry check
• Add test reproducing Deduplication inchi check vs aromaticity #255

Status

• Ready to go

[codecov]

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.

❗ 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]

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]

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)

• have the same InChi key
• are viewed by the toolkit as isomorphic when aromatic_matching=False
• are not viewed by the toolkit as isomorphic when aromatic_matching=True (the current behavior, and the toolkit's default)

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

[mattwthompson]

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

[j-wags]

Thanks for making the great test. I also had your confusion about the return value of ComponentResult.add_molecule, and I realized it was clearly a bug, since instead of returning a bool, it would return None, and assert(None) quacked like assert(False). I've fixed that and now everything seems hunky dorey, and I've added a little more testing for it as well. I've updated the release notes saying as much.

PS in a previous commend you said

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

and I'll clarify for future readers that it's a double bond/formal charge that's hopping around, not a proton!

[jthorton]

Great job guys!