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

Dihedral class #146

Merged
merged 18 commits into from
Oct 11, 2019
Merged

Dihedral class #146

merged 18 commits into from
Oct 11, 2019

Conversation

uppittu11
Copy link
Member

Added a basic dihedral and dihedral_type class.

@ahy3nz
Copy link
Collaborator

ahy3nz commented May 17, 2019

Lookin good -

Can you update dihedral with some of the recent eq and hash functions like in angle?

And some member_types in dihedral_type like in angletype

And either alongside this PR or for someone else to do, we'll have to update our documentation on potential terms now that we have dihedral functionality! #31

@mattwthompson
Copy link
Member

I wonder if we should try to get as much as we can working in this PR or focus on just having a well-behaved class for starters? For example, should we update the converters and writers (if any) here or in a subsequent PR?

@ahy3nz
Copy link
Collaborator

ahy3nz commented May 20, 2019

I think "well-behaved class for starters" is sufficient. I think updating converters or writers is probably a PR for another day

@mattwthompson
Copy link
Member

Check out some changes after #128. If my code is clear enough it should be somewhat legible what needs to be added, if not I can also explain in English terms

@ahy3nz
Copy link
Collaborator

ahy3nz commented Oct 4, 2019

Can you update this PR to reflect more-recent API changes? (see angle.py and angle_type.py)

@uppittu11
Copy link
Member Author

Ok. I think I updated everything, but there have been 51 commits to topology since I had last worked on this, so there are probably things I missed. Pytest is failing, but it doesn't look like it's a problem with the dihedral class (TestConvertMBuild.test_3_layer_compound).

Also, I didn't mess with adding dihedral functionality to from_parmed since it seems out of the scope of this PR and we should probably just get this merged first.

@uppittu11
Copy link
Member Author

Just kidding! The pytest failure was my bad. I accidentally deleted a line when merging with master. Everything should be working now

return hash(tuple(self.connection_members))


def _validate_four_partners(connection_members):
Copy link
Member

Choose a reason for hiding this comment

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

This is fine but a couple thoughts:

  1. This could probably just be a function that doesn't return anything; if it raises an exception it will get raised and if it doesn't raise an exception it just passes the argument back
  2. This could probably be superclassed up to Connection with the number of connections and the name of the subclass as arguments

@mattwthompson mattwthompson merged commit 393b08b into mosdef-hub:master Oct 11, 2019
@mattwthompson
Copy link
Member

Thanks Parashara!

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.

4 participants