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

Fix mass and charge in GSD writer; Add angles and dihedrals to GSD writer #680

Merged
merged 24 commits into from
Aug 3, 2022
Merged

Fix mass and charge in GSD writer; Add angles and dihedrals to GSD writer #680

merged 24 commits into from
Aug 3, 2022

Conversation

chrisjonesBSU
Copy link
Contributor

@chrisjonesBSU chrisjonesBSU commented Jul 22, 2022

This helps address #678 where the gsd writer was trying to perform math operations with NoneType objects.

Still need to add a unit test

Edit: Some other errors popped up, and I ended up totally revamping the gsd writer. It is writing out angles and dihedrals now.

Edit: I edited the title again to better reflect the changes by the PR. After talking with Cal in #678 there aren't any changes to default mass and charge values.

@chrisjonesBSU chrisjonesBSU added the WIP work in progress - do not merge label Jul 22, 2022
@chrisjonesBSU
Copy link
Contributor Author

This also contains another fix where the gsd writer was trying to call on t.atom_type and t.name when t1 and t2 were already the types or names.

@chrisjonesBSU chrisjonesBSU changed the title Set default mass and charge to 0.0 Set default mass and charge to 0.0; Add angles and dihedrals to GSD writer Jul 23, 2022
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 23, 2022

This pull request introduces 1 alert when merging a537c3e into 2970412 - view on LGTM.com

new alerts:

  • 1 for Unused import

@chrisjonesBSU chrisjonesBSU changed the title Set default mass and charge to 0.0; Add angles and dihedrals to GSD writer Fix mass and charge in GSD writer; Add angles and dihedrals to GSD writer Jul 25, 2022
@chrisjonesBSU
Copy link
Contributor Author

So, it looks like f-string formatting doesn't work when using it with warnings.warn?

@daico007
Copy link
Member

hmm, that should not be the case, I used f-string with warnings.warn in multiple occasions before. What's the error message are you getting?

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 25, 2022

This pull request introduces 3 alerts when merging 918425b into c336588 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

@chrisjonesBSU
Copy link
Contributor Author

chrisjonesBSU commented Jul 25, 2022

I'm not getting an error, but when running in a notebook, the variable name with brackets is in the warning statement rather than it actually performing the string formatting and printing the variable's value.

I think I am missing a couple f".." in the warning statements..oops

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #680 (847e4b0) into develop (0a11627) will increase coverage by 0.24%.
The diff coverage is 98.66%.

@@             Coverage Diff             @@
##           develop     #680      +/-   ##
===========================================
+ Coverage    91.22%   91.47%   +0.24%     
===========================================
  Files           63       63              
  Lines         5172     5217      +45     
===========================================
+ Hits          4718     4772      +54     
+ Misses         454      445       -9     
Impacted Files Coverage Δ
gmso/formats/gsd.py 98.62% <98.66%> (+3.62%) ⬆️
gmso/utils/geometry.py 88.88% <0.00%> (+11.11%) ⬆️
gmso/utils/sorting.py 100.00% <0.00%> (+100.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 25, 2022

This pull request introduces 3 alerts when merging 0d3f4f1 into c336588 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 25, 2022

This pull request introduces 3 alerts when merging 9d1d195 into c336588 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

@chrisjonesBSU
Copy link
Contributor Author

Before we merge this, let me test it out with some more compounds from mBuild and make sure the information the gsd file is correct and makes sense.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 26, 2022

This pull request introduces 3 alerts when merging 4d0d34a into c336588 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

@chrisjonesBSU chrisjonesBSU added ready for review and removed WIP work in progress - do not merge labels Jul 26, 2022
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 26, 2022

This pull request introduces 3 alerts when merging 2fbc265 into c336588 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 27, 2022

This pull request introduces 3 alerts when merging 36f30a4 into c336588 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

@chrisjonesBSU
Copy link
Contributor Author

The workflow in the latest unit test I added also solves mosdef-hub/mbuild#1025

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 29, 2022

This pull request introduces 3 alerts when merging 3ac2565 into cc23eb8 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 3, 2022

This pull request introduces 3 alerts when merging 2ec890a into 0a11627 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

Slight update to docstring, the rest LGTM!

gmso/formats/gsd.py Outdated Show resolved Hide resolved
Copy link
Contributor

@CalCraven CalCraven left a comment

Choose a reason for hiding this comment

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

LGTM! Would like some testing for more ways to write out GSD files and testing them, but until we have a more extensive set of things we want to be sure to support, I think we're good.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 3, 2022

This pull request introduces 3 alerts when merging 847e4b0 into 0a11627 - view on LGTM.com

new alerts:

  • 2 for Testing equality to None
  • 1 for Unused import

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

Successfully merging this pull request may close these issues.

None yet

4 participants