search

OpenFreeEnergy / feflow

Recipes and protocols for molecular free energy calculations using the openmmtools/perses and Open Free Energy toolkits
MIT License
13 stars 1 forks source link

Allow multiple mappings #90

Closed jthorton closed 2 months ago

jthorton commented 2 months ago

Fixes #89 using the same method as the relative hybrid topology method to select the first mapping if a list is provided.

pep8speaks commented 2 months ago

Hello @jthorton! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 991:80: E501 line too long (82 > 79 characters) Line 1004:80: E501 line too long (82 > 79 characters) Line 1007:80: E501 line too long (86 > 79 characters)

Line 339:80: E501 line too long (83 > 79 characters) Line 342:80: E501 line too long (83 > 79 characters) Line 518:80: E501 line too long (81 > 79 characters) Line 522:80: E501 line too long (88 > 79 characters)

Comment last updated at 2024-09-24 15:20:12 UTC
IAlibay commented 1 month ago

@ijpulidos were you ok with the changes in this PR?

@jthorton we hadn't communicated this (we need to document it) but because FEFlow aims to contain different Protocols by different folks, we aim to have the Protocol code owner (in this case Ivan), be the true approver if we can. Probably doesn't matter in this case, but something to try to strive for where possible.

jthorton commented 1 month ago

Ah sorry about that I thought the green tick was enough feel free to revert @ijpulidos!

ijpulidos commented 1 month ago

Yes, I think these changes look okay. It makes sense that the components are not exactly the same (but copies of it) in the tests, and that we can also explicitly handle list of mappings. Looks good to me!