haddocking / pdb-tools

A dependency-free cross-platform swiss army knife for PDB files.
https://haddocking.github.io/pdb-tools/
Apache License 2.0
390 stars 114 forks source link

Improve `pdb_merge` to include `TER`s and `END` lines #150

Open joaomcteixeira opened 2 years ago

joaomcteixeira commented 2 years ago

Closes #149

joaomcteixeira commented 2 years ago

That's right. However, I feel we are adding too much to the purpose of merge. All that should be done by tidy. Maybe if we exchange the -strict option for tidy we don't need to touch the merge. What do you think? Because the merge was really designed to be a concatenator

amjjbonvin commented 2 years ago

Yes - but why does it remove the END statements then? Other code don’t do that (e.g. pdb_chain).

joaomcteixeira commented 2 years ago

The current version of pdb_merge does not remove any lines from the input files. It's a straight concatenation.

https://github.com/haddocking/pdb-tools/blob/2a070bbacee9d6608b44bb6d2f749beefd6a7690/pdbtools/pdb_merge.py#L84-L87

amjjbonvin commented 2 years ago

So a simple cat command effectively.

Meaning pdb_tidy should always be run to correct things.

A very different behaviour than pdb_mkensemble for example.

joaomcteixeira commented 2 years ago

Yes. mkensemble has the clear purpose of making a correct ensemble of structures. While merge assumes the user knows what he/she is doing when concatenating the files. Likely the user cleaned the PDBs before using merge.

amjjbonvin commented 2 years ago

hmmm…. assumptions… this is asking for troubles…

It is this make clear to the users?

Yes. mkensemble has the clear purpose of making a correct ensemble of structures. While merge assumes the user knows what he/she is doing when concatenating the files. Likely the user cleaned the PDBs before using merge.

joaomcteixeira commented 2 years ago

Good point. Let's first clarify that to users before changing the behavior of pdb_merge.

joaomcteixeira commented 1 year ago

I was reviewing this now, and I still agree we should not touch pdb_merge for this purpose and pipe the results to pdb_tidy as the logic is not that straightforward. It is stated in the docs.

The contents are not sorted and no lines are deleted (e.g. END, TER
statements) so we recommend piping the results through `pdb_tidy.py`.
amjjbonvin commented 1 year ago

How will you explain users that pdb_merge does require pdb_tidy to make a correct PDB while pdb_mkensemble not...

joaomcteixeira commented 1 year ago

You are right. I am addressing this. It's not as straightforward as adding an END because also TER was not accounted for. If TERs exist in the output is because they were already present in the input. I am working on it.

joaomcteixeira commented 1 year ago

When doing pdb_merge, should atom numbers be renumbered starting from 1? This would change the original atom numbers but avoid repeated numbers.

amjjbonvin commented 1 year ago

Probably good indeed

joaomcteixeira commented 1 year ago

Hi @amjjbonvin

I have addressed this issue, plus some other details. Now, pdb_merge operates as follows:

Inside tests/data/ there are three PDBs, dummy_merge_A/B/C.pdb that you can use to test. Test also with others you may know, and let me know.