Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration tests #281

Merged
merged 26 commits into from Sep 3, 2020
Merged

Integration tests #281

merged 26 commits into from Sep 3, 2020

Conversation

pckroon
Copy link
Member

@pckroon pckroon commented Jun 26, 2020

Sets up integration tests based on files provided by @paulocts

Fixes #183

Will need cleanup and rebasing before it's ready for review.

@pckroon pckroon closed this Jun 26, 2020
@pckroon pckroon reopened this Jun 26, 2020
@pckroon pckroon marked this pull request as draft June 26, 2020 15:05
@fgrunewald
Copy link
Member

two small comments I noticed when doing the EN, which might be relevant here as well:

  • direct comparison of interactions won't work because martinize has these ifdef statements (i.e. meta is populated differently) and also the force-constant is set as variable

  • mapping of martinize1 and martinize2 is different so EN will not be the same. Generally for the martini2 stuff martinize1 is taken as reference as far as I understand

@pckroon
Copy link
Member Author

pckroon commented Aug 10, 2020

I rebased it to clean up the history a bit. It still requires 1 file (tier-1/bpti/martinize2/molecule_0.itp) to be checked by paulo

@pckroon pckroon marked this pull request as ready for review August 18, 2020 10:58
@pckroon pckroon requested a review from jbarnoud August 18, 2020 10:59
Copy link
Collaborator

@jbarnoud jbarnoud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a first round with the datafiles. I do not want to take the risk of loosing these comments is github fucks up.

@pckroon
Copy link
Member Author

pckroon commented Aug 21, 2020

Turns out the cg.pdb does not get written to the current working dir, but somewhere else. Investigating...

vermouth/tests/integration_tests/test_integration.py Outdated Show resolved Hide resolved
MARTINIZE2 = find_in_path()


def assert_equal_blocks(block1, block2):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried, and it became a mess: (pytest_assertion plugin: representation of details failed: .../lib/python3.6/site-packages/_pytest/assertion/util.py:377: ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all().. The only way to make it work would be to basically reimplement the pytest reporting machinery, which seems rather out-of-scope.
Similarly for the 'distance' attribute on edges, and interactions, these seem plagued by rounding issues, and I can't figure out a way to make the output remotely useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, adding a custom reporter for np.ndarray does not work. You'd need to get into the assertion rewriting, which is not possible pytest-dev/pytest#5347

@jbarnoud
Copy link
Collaborator

I do not find what you do with the files in the "martinize1" directories. Are you using them or did I miss something? Also, I think it would be good to separate what is expected to be the same between martinize 1 and 2 , and what is expected to be different. This way, you can make sure martinize 2 produces the same output as martinize 1 when it should.

This way the output pdb file ends up at the correct place: the temporary
directory. To enable rewriting to the data file path (e.g. for reading),
prepend a '.'
@pckroon
Copy link
Member Author

pckroon commented Aug 24, 2020

I removed the M1 directories. They were not used in the tests, and only in the hand crafting of the files in the M2 directories. Those files have been carefully vetted.
I don't see a feasible way of creating a custom pytest reporter for Molecule that gives me remotely comparable output to what I have now.

Peter Kroon added 6 commits August 24, 2020 15:39
- the first colinear dummy was always placed on the anchor
- Fix approx testing of coordinates in integration tests
- Fix approx testing of distances of edges in integration tests
- Remove positions from charge dummy nodes in integration tests, since those are (partially) random. The relative positions are tests by edge distances.
The removed rubber band got a distance of 0.92, which is more than the
maximum (0.9)
@pckroon pckroon requested a review from jbarnoud August 24, 2020 14:51
@pckroon
Copy link
Member Author

pckroon commented Aug 25, 2020

The low patch coverage is kind of expected, since the integration tests run in a separate interpreter, and the coverage they generate is not recorded.

Copy link
Member

@fgrunewald fgrunewald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. Few small comments double checking some things. Also before minting the new release we should include all interactions in the ff-parser and itp parser.

bin/martinize2 Show resolved Hide resolved
bin/martinize2 Show resolved Hide resolved
vermouth/processors/locate_charge_dummies.py Show resolved Hide resolved
@pckroon
Copy link
Member Author

pckroon commented Sep 3, 2020

Also before minting the new release we should include all interactions in the ff-parser and itp parser.

Disagree. A release is long overdue, and using not-yet-implemented interactions causes an understandable error message.

Copy link
Member

@fgrunewald fgrunewald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking out comments to issues this is good to go.

@pckroon pckroon merged commit c4471b4 into master Sep 3, 2020
@pckroon pckroon deleted the integration_tests branch September 3, 2020 09:46
@pckroon
Copy link
Member Author

pckroon commented Sep 3, 2020

Created #299 #300 #301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests need to run martinize2
3 participants