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

Automated Dihedral Analysis #217

Merged
merged 182 commits into from
Feb 12, 2023
Merged

Automated Dihedral Analysis #217

merged 182 commits into from
Feb 12, 2023

Conversation

cadeduckworth
Copy link
Contributor

@cadeduckworth cadeduckworth commented Oct 20, 2022

* separating functionality for use with _single_universe without iterating frames
* retaining functionality for use with _single_frame
* try/except pattern condensed to one run method
* single run method implemented for _single_universe and _single_frame
* functionality of original run method retained, while removing frame iteration for _single_universe usage
* progress bar still active when using _single_universe, but no frame iteration occurs
directory_paths: finds MDPOW project data and returns pandas DataFrame
directory_iteration: takes pandas DataFrame or csv and iterates automated_dihedral_analysis over MDPOW project directories
automated_dihedral_analysis: group of functions that includes automation scripts for DihedralAnalysis, automatic dihedral atom group selection, seaborn violin plots function, and other required functions
Future changes will eliminate redundancies following initial testing
…class to test possible ways to call automation-related functions
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Move all automation into a separate module: mdpow/analysis/workflows/base.py.

Add documentation. Need to include it in the html docs.

Add tests.

mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
mdpow/analysis/dihedral.py Outdated Show resolved Hide resolved
@orbeckst
Copy link
Member

This PR requires PR #216 to be merged, otherwise analysis runs twice.

cadeduckworth and others added 10 commits October 21, 2022 20:21
removed raise
added docstrings to describe use of NotImplementedError
responded with reason for removing _conclude_universe()
next step: updating html documentation following successful testing
fixed expected indent block error
removed raise
removed _conclude_universe()
…se automation for DihedralAnalysis

automated_dihedral_analysis.py
directory_iteration.py
directory_paths.py
@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #217 (ac8c8a7) into develop (729a6fb) will increase coverage by 0.90%.
The diff coverage is 93.22%.

@@             Coverage Diff             @@
##           develop     #217      +/-   ##
===========================================
+ Coverage    79.06%   79.96%   +0.90%     
===========================================
  Files           12       13       +1     
  Lines         1729     1847     +118     
  Branches       271      282      +11     
===========================================
+ Hits          1367     1477     +110     
- Misses         277      280       +3     
- Partials        85       90       +5     
Impacted Files Coverage Δ
mdpow/analysis/dihedral.py 100.00% <ø> (ø)
mdpow/workflows/dihedrals.py 93.22% <93.22%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@orbeckst
Copy link
Member

I'll look at this one again when #218 and #216 are addressed.

@orbeckst
Copy link
Member

For test files: Create a set of data with XTC files that only contain ~10 frames (evenly spaced along the trajectory). Include only TPR files necessary for the test (eg only Coulomb lambda 0).

@orbeckst
Copy link
Member

@cadeduckworth please resolve conflicts now that PR #216 has been merged. This should get this PR up-to-date.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

TODO

  • Move all automation into a separate module: mdpow/analysis/workflows ; have a base.py for general stuff and a dihedrals.py for dihedrals specific
  • Add documentation. Show how one uses your code. Use test files as examples so that someone could directly try it out. These howto-docs also give you an idea how to organize and structure.
  • Add tests. In particular, create a minimal test set. Automated Dihedral Analysis #217 (comment)

@cadeduckworth
Copy link
Contributor Author

Some issues need to be unlinked and linked to separate or new PR's after this one is merged

@cadeduckworth
Copy link
Contributor Author

@orbeckst
I will double check docs, tests, and modules, and update if I notice anything, but it appears that everything should be ready for final review.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Good changes. I found one incorrect assertion, some formatting issues, and please clean up comments/commented out code and make some of the comments more specific so that people after you (or your future self) has a chance understanding what the problem is.

mdpow/tests/test_automated_dihedral_analysis.py Outdated Show resolved Hide resolved
mdpow/tests/test_automated_dihedral_analysis.py Outdated Show resolved Hide resolved
mdpow/tests/test_automated_dihedral_analysis.py Outdated Show resolved Hide resolved
assert groups[i].all() == self.check_groups[i].all()
i+=1

@pytest.mark.skipif(sys.version_info < (3, 8), reason="scipy circvar gives wrong answers")
Copy link
Member

Choose a reason for hiding this comment

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

Add issue number, add more text to be specific ... whoeever reads this has no clue what "nature of this problem" means.

Read the comments through the eyes of someone who is not an insider.

mdpow/tests/test_automated_dihedral_analysis.py Outdated Show resolved Hide resolved
mdpow/workflows/dihedrals.py Outdated Show resolved Hide resolved
mdpow/workflows/dihedrals.py Outdated Show resolved Hide resolved
mdpow/workflows/dihedrals.py Outdated Show resolved Hide resolved
mdpow/workflows/dihedrals.py Outdated Show resolved Hide resolved
mdpow/workflows/dihedrals.py Show resolved Hide resolved
@cadeduckworth
Copy link
Contributor Author

@orbeckst All requested changes have been addressed, and relevant or meaningful inline comments that do not fit the scope of this pull request have been moved to new or existing issues.

As a reminder (because I tried, but could not figure out how to do it), issues 232 and 235 need to be unlinked to make sure they are not closed with this PR. Only issue 236 should be closed with this PR.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

excellent work, good to go 🎉

I'll squash-merge.

@orbeckst
Copy link
Member

oops, need entry in CHANGES ... will do that before merge

@orbeckst orbeckst merged commit 3faba4f into develop Feb 12, 2023
@orbeckst orbeckst deleted the automated-dihedral-analysis branch February 12, 2023 03:42
@orbeckst
Copy link
Member

Congratulations @cadeduckworth , your big PR is in MDPOW! 🎉 🍾

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 appropriate error, exception, logging methods for these patterns, throughout
2 participants