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

Creates AnalysisBase function PropkaTraj #26

Merged
merged 18 commits into from
Jul 9, 2020

Conversation

IAlibay
Copy link
Collaborator

@IAlibay IAlibay commented Jul 8, 2020

Supersedes #25
Fixes #23, #19, and temporarily addresses #24

This ended up a relaxing enough weekend thing to do...

Changes made:

  • Created a new AnalysisBase class named PropkaTraj which essentially has the same functionality as get_propka.
  • Major differences from get_propka include;
    • temporarily files are now written to the current directory (in the future we should consider moving this to an actual temporary directory).
    • if a bad frame is identified but skip_failure is True, then a warning is thrown, but the statistics about the number of bad frames still users the logger (can go either way here, I just thought you'd definitely want to warn users they lost a frame, but statistics is more of a log thing).
  • get_propka has now been deprecated.
  • Both get_propka and PropkaTraj now warn users if they don't pass purely protein residues as a selection. This can be removed once the MDAnalysis writer is fixed (should be easy enough to do, but I can't seem to find a good reference for the full list of "standard residues" ).
  • Adds test to cover trajectories with bad frames and various warnings (coverage is now 100%).
  • Updated the notebook, README, and INSTALL instructions to reflect the new class.

Questions:

  • I've set the next version as 1.0.3 and the deprecation to occur in 2.0.0. My thought here is that if the next version is propka 3.2 compatible, you might want to bump up the version number by a major number. That can be changed though.

@@ -2,22 +2,239 @@
# Copyright (c) 2013-2017 David Dotson, Ricky Sexton, Armin Zjajo, Oliver Beckstein
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the copyright notice need to be changed? I know in the past you've added authors to it, but I thought it be better to ask.

Copy link
Member

Choose a reason for hiding this comment

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

add yourself & change the year, please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have a preferred ordering? It's not alphabetical, the closest I could find was that it was ordered by number of commits, but I guess it's more "first author" "corresponding author"?

Also should Shujie Fan be added too?

Copy link
Member

Choose a reason for hiding this comment

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

You're thorough :-) – and thanks for remembering Shujie.

Let's do David, Irfan, Ricky, Armin, Shujie, Oliver.

@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #26 into master will increase coverage by 22.91%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           master       #26       +/-   ##
============================================
+ Coverage   77.08%   100.00%   +22.91%     
============================================
  Files           2         2               
  Lines          48       103       +55     
============================================
+ Hits           37       103       +66     
+ Misses         11         0       -11     
Impacted Files Coverage Δ
propkatraj/__init__.py 100.00% <100.00%> (ø)
propkatraj/propkatraj.py 100.00% <100.00%> (+23.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e15b206...d83c602. Read the comment docs.

@IAlibay IAlibay mentioned this pull request Jul 8, 2020
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.

Awesome!

Minor things in comments – mainly that the next release will be 1.1.0 instead of 1.0.3.

propkatraj/propkatraj.py Outdated Show resolved Hide resolved
propkatraj/propkatraj.py Outdated Show resolved Hide resolved
propkatraj/propkatraj.py Show resolved Hide resolved
propkatraj/propkatraj.py Outdated Show resolved Hide resolved
propkatraj/propkatraj.py Outdated Show resolved Hide resolved
@IAlibay
Copy link
Collaborator Author

IAlibay commented Jul 9, 2020

Thanks for the review @orbeckst, that should be everything addressed.

@orbeckst orbeckst merged commit df80686 into Becksteinlab:master Jul 9, 2020
@orbeckst
Copy link
Member

orbeckst commented Jul 9, 2020

Cheers!

@IAlibay IAlibay deleted the new-analysisbase branch July 9, 2020 08:02
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.

get_propka will fail with file generated via fetch_mmtf
2 participants