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

Initial tests for writing functions #31

Merged
merged 8 commits into from
Oct 28, 2023

Conversation

CalCraven
Copy link
Collaborator

This PR aims to add tests for all of the utility readers and writers used in the repo. To aid in this testing, a set of files are all stored in tests/files/ for ease of access and testing on.

  • Add files output from gaussian
  • Fix a few typos for guass used instead of gauss
  • Finish tests that are just set to pass for now

@bc118
Copy link
Collaborator

bc118 commented Oct 11, 2023

Notes: HEATOM is not supported in mosdef-GOMC writers, it always writes ATOM in pdb files

Copy link
Collaborator

Choose a reason for hiding this comment

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

ATOM is currently the only output for mosdef-GOMC pdb files.

@bc118
Copy link
Collaborator

bc118 commented Oct 11, 2023

Should we put all these test files in the utils/files (https://github.com/GOMC-WSU/MoSDeF-dihedral-fit/tree/main/mosdef_dihedral_fit/utils/files), where all the other ones are?

Maybe I don’t understand the reason.

@CalCraven
Copy link
Collaborator Author

Should we put all these test files in the utils/files (https://github.com/GOMC-WSU/MoSDeF-dihedral-fit/tree/main/mosdef_dihedral_fit/utils/files), where all the other ones are?

In terms of packaging, typically you want files that are solely there for test validation to exist in the /tests module. /utils/files would be something more like file templates or something that is actually used during the course of operating the package.

@CalCraven
Copy link
Collaborator Author

Looks like macos and ubuntu tests are giving different outputs for the fifth element of the dihedral angle (macos=+180.0, ubuntu=-180.0). Probably something with the get_matching_dihedral_info_and_opls_fitting_data

Looks like maybe a bug in this line:

  if dihedral_angle_degrees == 180:
      dihedral_angle_degrees == -180

Should be?

  if dihedral_angle_degrees == 180:
      dihedral_angle_degrees = -180

Regardless, I'm guessing the difference is coming from a dot or cross product direction.

@bc118
Copy link
Collaborator

bc118 commented Oct 23, 2023

Looks like macos and ubuntu tests are giving different outputs for the fifth element of the dihedral angle (macos=+180.0, ubuntu=-180.0). Probably something with the get_matching_dihedral_info_and_opls_fitting_data

Looks like maybe a bug in this line:

  if dihedral_angle_degrees == 180:
      dihedral_angle_degrees == -180

Should be?

  if dihedral_angle_degrees == 180:
      dihedral_angle_degrees = -180

Regardless, I'm guessing the difference is coming from a dot or cross product direction.

Yeah, I agree, you are likely correct here

@CalCraven
Copy link
Collaborator Author

ATOM is currently the only output for mosdef-GOMC pdb files.

That is fine, I think we can support HETATM since that's what you get from MoSDeF pdb writers and as far as I can tell the data can get parsed the same way.

@CalCraven CalCraven force-pushed the test_read_write_functions branch from 9517dbd to b2d219b Compare October 27, 2023 19:33
Copy link
Collaborator

@bc118 bc118 left a comment

Choose a reason for hiding this comment

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

LGTM

Since we created the same files again in "test/files" (used for unit tests). Let's remove all the files from "utils/files/" and move them to "test/files", in a quick next PR. This way all the test files are in 1 location.....

@bc118 bc118 merged commit 45a1033 into GOMC-WSU:main Oct 28, 2023
5 checks passed
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.

2 participants