-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add Pylint #55
Add Pylint #55
Conversation
Address pylint rules W0611, W0102, W1401, E1120, C0121, C0123, C0325, R0124, R1714.
Address pylint rules W0311, W1309.
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.
You can find a list of Pylint's rules here.
Due to the large number of changes, and the fact that some changes affect scripts that are not covered by the testsuite, I would recommend allocating more than 1 code reviewer to this PR.
Address Pylint rules W0123, R1722.
I have been checking around, testing the parts that are not covered by the CI and I did not find any further issue with the PR. Still, it would be good if at least @paobtorres or @davidbbeyer checks as well before I merge this PR |
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.
Apart from the small points raised above, it looks good to me.
@@ -39,7 +34,7 @@ | |||
|
|||
# Peptide parameters | |||
|
|||
sequence = 'nEEEEEc' | |||
sequence = 'EEEEDDDD' |
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.
What is the reason for getting rid of the termini 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.
@davidbbeyer the reason is that the termini were actually not parametrized in parameters/peptides/Lunkad2021.json
. In earlier versions of this script, this was bypassed by adding a generic parametrization for the non-parametrized aminoacid particles. This was a convenient solution to allow to run any peptide sequence, but not really a good practice (since we were basically teaching people to mix parameters instead of thinking about what should be a good parametrization). With @mariusaarsten we have done a more complete parametrization of the amino acids which he will add in his master thesis and we will also provide in pyMBE so we can have a first complete set of parameters for all aminoacids.
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.
@davidbbeyer @paobtorres I think we are ready now for merging this PR. If you agree, please leave a review approving these changes (go to the files changes tab, review changes botton, and press approve). @paobtorres please open a dedicated PR to fix the issues that you spotted in the create_paper_data.py
script.
Description of changes:
assign_molecule_id()
and sample scriptspdoc
warnings in CI