added Forcefield.apply assert_bonds

[richardjgowers]

No description provided.

[mattwthompson]

Thanks for the contribution!

My understand was that it's supposed to get caught earlier (see here) but I know you're dealing with topologically different systems than we were using back then. I'm a little fuzzy on this so I will let @summeraz comment.

[richardjgowers]

It looks like that comment says it gets picked up in the MD engine later on. Arguably this is a thing for foyer to spot.

Looks like this change breaks an existing test, I think you all can push to this branch to fix

[summeraz]

Thanks @richardjgowers! So the reason this wasn't added in #155 was because (at least at that time) if parameters were not provided in the Foyer XML for all bonds, an error was raised by OpenMM within createSystem. Can you add a unit test for what you've added here? My guess is that things will break before reaching the _error_or_warn line you've added.

[richardjgowers]

@summeraz I've added a test, it's possible to do Forcefield.apply() and get bonds with no parameters

[mikemhenry]

Why did this PR get merged in with a failing test?

test_surface FAILED 
E           Exception: Parameters have not been assigned to all bonds. Total system bonds: 2303, Parametrized bonds: 2178

[mattwthompson]

Oops, that was sloppy. We thought it was one of the tests that can fail for silly reasons, but on further inspection it's actually something we need to change. Thanks for catching this

[mikemhenry]

No worries! I was just surprised that while I was working on another PR, that test started to fail. It looks like this PR was really needed, since one of the tests didn't have all the bonds defined.

[mattwthompson]

Yeah, of all the things that can happen with failing units tests, finding and fixing an unrelated issue is among the better outcomes