search

galaxycomputationalchemistry / galaxy-tools-compchem

:mega: Galaxy Tools for Computational Chemistry
Apache License 2.0
14 stars 16 forks source link

Fix bug ordering trajectory inputs in trjcat tool #153

Closed simonbray closed 2 years ago

simonbray commented 2 years ago

In #148 we discussed the problem of ordering trajectories for the trjcat tool. I believe the tool is in general already able to do this, if a collection is used as the input. There is a bug though; the tool iterates over inputs and symlinks to traj_0.xtc, traj1.xtc ... If there are more than 10 inputs, ordering is broken, as the trjcat command will place traj10.xtc between traj1.xtc and traj2.xtc.

To fix, we can simply use Python's rjust to pad 10 leading zeroes to the file index.

IIRC I was already aware of this bug when I wrote the tool 2-3 years ago, but couldn't work out how to add the leading zeroes in Cheetah :( In the end I just gave up and added the Please note this tool does not currently take order into account disclaimer. (For my own use case at the time, order wasn't that important.)

ping @thepineapplepirate

thepineapplepirate commented 2 years ago

so my understanding is that the changes in PR #148 allow for what you are proposing here, just perhaps in a slightly different way. currently, #148 enables collection based ordering and specific individual ordering, as well as multiple collection based ordering. both collections and individual trajectories can be used as an input, so more options/versatility there.

my main reservation here, is that this PR would require the user to take an additional step - making a collection out of their trajectories, before using the tool. it seems more intuitive for users to just be able to use what's in their history already. that's just my opinion :). also, i noticed this PR doesn't include '-skip' as an option under trjconv. This is sort of a vital functionality, as its the only place in gmx where a user can shorten these trajectories.

simonbray commented 2 years ago

so my understanding is that the changes in PR #148 allow for what you are proposing here, just perhaps in a slightly different way. currently, #148 enables collection based ordering and specific individual ordering, as well as multiple collection based ordering. both collections and individual trajectories can be used as an input, so more options/versatility there.

my main reservation here, is that this PR would require the user to take an additional step - making a collection out of their trajectories, before using the tool. it seems more intuitive for users to just be able to use what's in their history already. that's just my opinion :). also, i noticed this PR doesn't include '-skip' as an option under trjconv. This is sort of a vital functionality, as its the only place in gmx where a user can shorten these trajectories.

I didn't mean to present this PR as an alternative to #148, sorry if I created that impression. This is just to fix a bug in the existing functionality of the tool.

thepineapplepirate commented 2 years ago

so my understanding is that the changes in PR #148 allow for what you are proposing here, just perhaps in a slightly different way. currently, #148 enables collection based ordering and specific individual ordering, as well as multiple collection based ordering. both collections and individual trajectories can be used as an input, so more options/versatility there. my main reservation here, is that this PR would require the user to take an additional step - making a collection out of their trajectories, before using the tool. it seems more intuitive for users to just be able to use what's in their history already. that's just my opinion :). also, i noticed this PR doesn't include '-skip' as an option under trjconv. This is sort of a vital functionality, as its the only place in gmx where a user can shorten these trajectories.

I didn't mean to present this PR as an alternative to #148, sorry if I created that impression. This is just to fix a bug in the existing functionality of the tool.

no worries man :).