Fix #234

[j-wags]

Description

In openforcefield/openff-toolkit#1498 we introduced helpful logic to reindex the keys for any values in offmol.properties['atom_map'] so that they correspond to the new ordering. It turns out that BespokeFit ALSO did this, leading to a double-reindexing, which the tests caught and are warning us about. This can be fixed by removing the now-redundant logic from BespokeFit.

Todos

• Resolve Failing tests #234
• Update releasehistory

Status

• Ready to go

[codecov]

Codecov Report

Merging #235 (91c0875) into main (1b87638) will decrease coverage by 0.95%.
The diff coverage is 100.00%.

Additional details and impacted files

[mattwthompson]

Naively this looks like the fix for #234?

[j-wags]

Hard to tell if this is a case of too much coffee or too little 🤦

[jthorton]

Great catch @j-wags this makes sense to me!

[Yoshanuikabundi]

Thanks @j-wags! This is a good catch, but I think we need a slight tweak to support both Toolkit 0.11 and 0.12.

[mattwthompson]

If both minor versions of the toolkit must be supported, should those be added as separate versions in CI?

[j-wags]

I've added the suggested changes - I think this should be good for another look now!

Re: Testing against earlier versions - yeah, that'd be nice, but as you imply it's also be impractical. This solution seems elegant to me because it simply overwrites the changed behavior with exactly the desired logic. I'd trust this to work on the old version without adding CI testing.

I'd have been slightly in favor of pinning openff-toolkit>=0.12.1 in the package recipe, but I'm not the owner and this is also a fine resolution.

[Yoshanuikabundi]

Perfect, thanks!

I feel like supporting 0.11 and 0.12 just keeps our options open.

[Yoshanuikabundi]

Hey @j-wags is there something holding this up? It's blocking testing on a number of other PRs so I think I'll merge it myself tomorrow if I don't hear back :)

[Yoshanuikabundi]

The integration tests will be fixed in #243 so don't worry about green checks :)

[j-wags]

Sorry for the delay - merged!