marrink-lab / vermouth-martinize

Describe and apply transformation on molecular structures and topologies
Apache License 2.0
84 stars 37 forks source link

Bug: check DSSP version #505

Closed juminlee closed 1 year ago

juminlee commented 1 year ago

Hello.

It seems like that there is a bug in dssp.run_dssp().

https://github.com/marrink-lab/vermouth-martinize/blob/6d609995078f6194f560680ba0ccbf483d5ef825/vermouth/dssp/dssp.py#L197

Isn't this line supposed to be: process = subprocess.run([executable, "--version"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)

pckroon commented 1 year ago

Hi,

it should absolutely be! Thanks for spotting this. Would you be willing to open a PR with this change? Otherwise I'll do it myself somewhere the coming days, but I'm rather busy...

juminlee commented 1 year ago

No problem. I have one more related question. Does vermouth only support DSSP 3.0.0? I tried DSSP 2.2.1, and it worked fine. If vermouth works fine for DSSP 2 and 3, then it might be better to change a code a bit to allow all DSSP 2 and 3 versions.

pckroon commented 1 year ago

Depending on how much work/code change it would be. I'm a bit hesitant to support old/deprecated versions, let alone promise that, especially since the development/maintenance team is rather small.

Having stated this, please investigate how much work it would be! I trust your judgement on whether it's worth the effort. It would definitely make the whole martinize2 experience smoother...

juminlee commented 1 year ago

If you are OK to support all DSSP 2 and 3, the code change is already done in my end. If you want to support DSSP 3 as a default, and allow DSSP 2 only for a special request as a function argument, I may need to change couple more lines.

pckroon commented 1 year ago

Great! I suggest you just open the PR then. That will give me a better idea of how much you had to change, and how much work support is going to be. We can continue discussing the specifics there.

juminlee commented 1 year ago

Created pull request: #506

pckroon commented 1 year ago

Fixed in #506