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

updating open babel energy minimization to no longer use temp file #1078

Merged
merged 27 commits into from
Feb 2, 2023

Conversation

chrisiacovella
Copy link
Contributor

PR Summary:

This is related to issue #1077. Rather that relying on a temporary .mol2 file to perform open babel energy minimization, this PR will directly set the relevant parameters in the open babel OBMol class, and extract energy minimized coordinates from this class for updating of the positions in the Compound.

My initial plan was to simply store Compound.name information, overwrite it with Compound.element before generating the .mol2 file, and then revert Compound.name parameters to the original entries. However, using the API directly to set the info seems like it would be better with less overhead.

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.54%. Comparing base (042f814) to head (40f4297).
Report is 129 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1078   +/-   ##
=======================================
  Coverage   90.54%   90.54%           
=======================================
  Files          61       61           
  Lines        6186     6190    +4     
=======================================
+ Hits         5601     5605    +4     
  Misses        585      585           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chrisiacovella
Copy link
Contributor Author

chrisiacovella commented Jan 22, 2023

linux tests are failing...appears related to try to access atomic number. I think I need to check to make sure element is properly set.

The bit of code in the energy_minimize function that tries to infer atom type from name, never actually set the element field with the atom info that was inferred. Not sure how the tests were every passing before.

@chrisiacovella
Copy link
Contributor Author

All tests are now passing after a quick change of the test_energy_minimize_non_element function to reflect the changes in the code itself.

Interestingly, the incorrectly written test passed on macOS (both my local machine running pytest and CI), but appropriately failed on linux. The incorrectly written test did not raise an MBuildError like it should, but pytest didn't consider that a failure for some reason.

I'm not seeing anything wrong in the syntax...maybe just a weird bug and I guess good that linux caught it.

 with pytest.raises(MBuildError):
            octane.energy_minimize() 

@chrisiacovella
Copy link
Contributor Author

I found a minor bug in the to_gmso function in the compound class. kwargs are not passed to the function conversion.to_gmso. This is detailed in #1081

chrisiacovella and others added 2 commits January 31, 2023 11:00
… feature; changed function call and added todo for when feature is implemented
@chrisiacovella
Copy link
Contributor Author

Tests were failing when I had to_gmso() properly pass kwargs, because tests were set up to handle the yet to be implemented infer_hierarchy (without passing kwargs infer_hierarchy would be ignored). I added in a TODO note to change the function call of the tests back when this is implemented.

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.

I think a good chunk of conversion stuff already existed in the to_pybel method, which I think should be used as the base here (fix/add new features as needed). The conversion back looks good.

mbuild/compound.py Outdated Show resolved Hide resolved
mbuild/compound.py Outdated Show resolved Hide resolved
chrisiacovella and others added 5 commits February 1, 2023 09:39
I forget to delete this when I modified it.

Co-authored-by: Co Quach <43968221+daico007@users.noreply.github.com>
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.

I think this is looking pretty much done.

mbuild/compound.py Show resolved Hide resolved
mbuild/tests/test_gmso.py Outdated Show resolved Hide resolved
mbuild/tests/test_gmso.py Outdated Show resolved Hide resolved
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.

LGTM!

Christopher Iacovella added 7 commits February 1, 2023 13:16
@chrisiacovella
Copy link
Contributor Author

Read the docs is failing, but that can be addressed separately.

@chrisiacovella chrisiacovella merged commit 3d7c93d into mosdef-hub:main Feb 2, 2023
@chrisiacovella chrisiacovella deleted the e_min branch February 2, 2023 02:47
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.

3 participants