-
Notifications
You must be signed in to change notification settings - Fork 17
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
code and unit test for salt bridge tool #121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some more feedback for your salt bridge tool. Let me know if you have questions!
…altbridge_code
this branch has some conflicts -- can you resolve it? (see "Resolve conflicts" below). I think it's from merging main after we organize tools in alphabetical order. |
It is ready and fixed after some polishing ups. Can someone please review it so I can see my very first tool officially joining the club? 🥇 |
@brittyscience is this ready for review? |
…altbridge_code
…saltbridge_code
Co-authored-by: Sam Cox <swrig30@ur.rochester.edu>
Co-authored-by: Sam Cox <swrig30@ur.rochester.edu>
Co-authored-by: Sam Cox <swrig30@ur.rochester.edu>
|
||
def plot_salt_bridge_counts(self): | ||
if not self.salt_bridge_data or self.traj.n_frames == 1 or self.specific_frame: | ||
print("i'm here") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha remove debugging prints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, just remove the debugging printing statements before merging 👍
A fair warning, this branch failed commit, I told it to ignore the pre commit hooks so I can get the code pushed to git hub. This tool is still a work in progress.