jensengroup / propka

PROPKA predicts the pKa values of ionizable groups in proteins and protein-ligand complexes based in the 3D structure.
http://propka.org
GNU Lesser General Public License v2.1
263 stars 58 forks source link

Update with str.format #44

Closed sobolevnrm closed 4 years ago

sobolevnrm commented 4 years ago

The amount of changes is a bit overwhelming for a review, so I'm afraid I can only make some general comments.

I see a lot of variable and method name changes, and even a config name change (max_intrinsic_pka_diff). I suppose PROPKA 3.1 API compatibility is not a goal of this refactoring, and this will become PROPKA 4.0 when refactoring is complete? If this is a conscious decision, then I see no issue with that.

For multi-line statements, I think PEP8 prefers parentheses around the expression instead of a backslash, if possible.

Python 3.5 supports str.format which is a bit more "modern" than %-formatted strings, and uses the same syntax like the Python 3.6 f-strings.

Originally posted by @speleo3 in https://github.com/jensengroup/propka-3.1/pull/40#issuecomment-633659044

sobolevnrm commented 4 years ago

@speleo3 - where would you define the format strings: at the top of the module (as constants), within the class (as attributes), or immediately next to the code where they're used? The first two options might make it easier to test but could generate confusion as well.

speleo3 commented 4 years ago

Usually just immediately next to the code where it's used. Unless it's a long or complex format template, then I would put it as a constant at the module level.

sobolevnrm commented 4 years ago

@speleo3 - I'll also include module-level constants when the format needs to be documented for others' awareness (e.g., the standard pKa format of %6.2f that's used throughout PROPKA).

sobolevnrm commented 4 years ago

Hi @speleo3 - can you please look at https://github.com/Electrostatics/propka-3.1/commit/bcccd89ca4e9640a28a38bc4b8728203a9a353e0 ? This is an initial implementation in atom.py. I checked the results for a variety of residue types so would mainly like your thoughts on the implementation before I go much further.

speleo3 commented 4 years ago

Looks good! Only the newline characters, I would add them to the templates.

sobolevnrm commented 4 years ago

Fixed by dde0d67ea5d7b0e8b24a3050ae4d6cbc4ca07972