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

Polymer class revamp #851

Merged
merged 72 commits into from
May 20, 2021
Merged

Conversation

chrisjonesBSU
Copy link
Contributor

@chrisjonesBSU chrisjonesBSU commented Mar 22, 2021

PR Summary:

For some background, I'm currently working on a project where we need to easily initialize polydisperse polymer systems, so I spent quite a bit of time playing around with polymer.py. The Polymer class worked great at building up polymers from monomers, but in order to automate the process it required some work "preparing" the monomers before using Polymer() then some steps to "finish" the polymer compounds (basically putting back any missing hydrogens).

So, I thought it might be worth a shot to implement these "pre" and "post" polymer steps into the Polymer class itself while making it functional for just about any monomer compound that's initialized from either a SMILES string or a file.

I included a jupyter notebook that shows an example of how to go through the process and describing what is happening. It's currently located at mbuild/mbuild-polymer-example.ipynb. I wasn't sure if there was a better way to attach or include an interactive notebook, so I just included it in the PR. If this gets merged, I'll make sure to remove the notebook and make a separate PR to the tutorials repo.

Also, I merged in the changes from #839 to this PR as I needed to use some of the functionality from it.

I'm interested in any feedback, I imagine this is more in the rough draft stages than near completion, but hopefully it's a start to making it easier to build-up polymers.

PR Checklist


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

chrisjonesBSU and others added 30 commits February 2, 2021 22:51
@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2021

This pull request introduces 3 alerts when merging a06bc72 into 29e4699 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable

@chrisiacovella chrisiacovella self-requested a review April 26, 2021 18:04
@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2021

This pull request introduces 3 alerts when merging 04138b9 into 1ca3ff8 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2021

This pull request introduces 3 alerts when merging 4bf8660 into 45e5369 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2021

This pull request introduces 3 alerts when merging cdfd80d into 45e5369 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2021

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

new alerts:

  • 2 for Unused import
  • 1 for Unused local variable

Copy link
Contributor

@jennyfothergill jennyfothergill left a comment

Choose a reason for hiding this comment

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

I've played with this PR a lot an am a fan. c: I suggest one minor change.

Comment on lines 156 to 158
if last_part is None:
first_part = this_part
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

first_part is never used. I think we can simplify this logic to:

Suggested change
if last_part is None:
first_part = this_part
else:
if last_part is not None:

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2021

This pull request introduces 1 alert when merging 883f25f into a185759 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2021

This pull request introduces 1 alert when merging ed10425 into a185759 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented May 18, 2021

This pull request introduces 1 alert when merging 2608b9d into ba793dd - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@chrisjonesBSU
Copy link
Contributor Author

FYI, for anyone that still wants/needs to review this, I made a PR in the mbuild-examples repo that goes over the various ways to build polymers.

mosdef-hub/mbuild-examples#6

Copy link
Contributor

@justinGilmer justinGilmer left a comment

Choose a reason for hiding this comment

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

Based on the example you added in mbuild-examples and the notebooks and tests here, this looks like it is ready to go!

Copy link
Contributor

@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.

I approve with my requested changes

@justinGilmer justinGilmer merged commit 219207b into mosdef-hub:master May 20, 2021
@chrisjonesBSU chrisjonesBSU deleted the feat/polymer branch July 6, 2021 04:17
umesh-timalsina referenced this pull request in GOMC-WSU/MoSDeF-GOMC Mar 22, 2022
* added a couple new functions, moved existing code into build function

* added some doc strings

* added a default value to the port labels

* reworked the _add_port function

* saving my progress with the add_end_groups function

* made progress with end group funcitonality

* removing a couple print statements

* added 2 new attributes to port class

* removed changes from port.py, commented out orientation and separation lines

* changed method of removing hydrogens, added n_monomers var for finding last monomer

* adding to the doc strings

* added update_separation function

* update orientation func; separation property func

* added the up subports to the rotations

* fixed the change_orientation function

* fix handling when their is no anchor particle

* fix typo

* docstring: separation is None when port has no anchor

* added 3 unit tests

* remove the port updating stuff I was trying to use

* updating port separations for end groups

* fix the way clone is being called

* added some doc strings and comments

* updated doc strings

* including jupyter notebook example and walk through

* example notebook ready

* added to recipe examples

* expanded end_group options

* update alkane recipe to make tests pass

* cleaned up if statements when updating head/tail ports

* remove trailing whitespace

* allow use of compound with existing ports to be specified in init

whitespace/formatting

* update alkane to new schema

* update tests

* blacked, changed how head/tail port work

* remove debug print

* update notebook

* update notebook

* clear nb

* add back add_hydrogens

* add add_hydrogens=False

* add_hydrogens=False, all tests pass

* changed orientation of down port to face opposite direction of up port

* Few more examples added to show new functionality

* adding a few unit tests

* removing the example notebook from the PR

* test for errors, and manually passing in end group compounds

* expanded doc strings with more detailed explanations, and some examples

* fix a conflict that was sticking around in the port doc strings

* fix formatting in the doc strings

* Doc strings to explain implementation of sequence in the build function

* add unit test for value of n < 1

* 2 new tests to test for errors

* remove unused imports

* oops, typo

Co-authored-by: chrisjonesBSU <chrisjones4@u.bosiestate.edu>
Co-authored-by: Jenny <39961845+jennyfothergill@users.noreply.github.com>
Co-authored-by: Jenny Fothergill <jennyfothergill@u.boisestate.edu>
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.

Add option to cap chains created by mb.Polymer
6 participants