Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read chainID from PDBQT #4284

Merged
merged 5 commits into from
Sep 16, 2023
Merged

Conversation

pgbarletta
Copy link
Contributor

@pgbarletta pgbarletta commented Sep 6, 2023

Fixes #4207

The user also complained about lacking elements, but given that the PDBQT format replaced the element field from the PDB ATOM record, I guess is fair that we don't try to create the from the atomtype. In any case, the solution given to the user could be made a FAQ (if it's indeed a FAQ).

Changes made in this Pull Request:

  • Use the chain ID read from the atoms to create the proper chainIDs attribute instead of just discarding them.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4284.org.readthedocs.build/en/4284/

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Linter Bot Results:

Hi @pgbarletta! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2197a2c) 93.40% compared to head (e76db8b) 93.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##           develop    #4284     +/-   ##
==========================================
  Coverage    93.40%   93.40%             
==========================================
  Files          170      184     +14     
  Lines        22255    23364   +1109     
  Branches      4071     4071             
==========================================
+ Hits         20788    21824   +1036     
- Misses         951     1024     +73     
  Partials       516      516             
Files Changed Coverage Δ
package/MDAnalysis/topology/PDBQTParser.py 100.00% <100.00%> (ø)

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pgbarletta for opening this PR! I assume the PDBQT format doesn't include a segment column, or else that would be preferenced for segids? I was worried about what the PDBQT writer would do if the chainIDs and segids conflict -- looks like it tries to write chainIDs first, and then uses the last (not first...) letter of the segids.

There's a test for the segid attribute:

def test_segid(self, universe):
sel = universe.select_atoms('segid A')
assert_equal(sel.n_atoms, 909, "failed to select segment A")
sel = universe.select_atoms('segid B')
assert_equal(sel.n_atoms, 896, "failed to select segment B")

Would it be possible to add one for chainIDs too? Otherwise LGTM.

@pgbarletta
Copy link
Contributor Author

I assume the PDBQT format doesn't include a segment column, or else that would be preferenced for segids?

PDBQT has the concept of chainID, but not of segments. Taken from the PDBQT reference:

ATOM%5d%-4s%1s%-3s%1s%4d%1s%8.3f%8.3f%8.3f%6.2f%6.2f%4s%6.3f%2s \n",
atom_serial_num, atom_name, alt_loc, res_name, chain_id, res_num, ins_code, x, y, z,
occupancy, temp_factor, footnote, partial_charge, atom_type

I've seen the occupancy and temp_factor columns used for vdW and Elec, but never the footnote for segments, though I don't have that much experience with this format.

Would it be possible to add one for chainIDs too?

Done. Didn't add it at first since both segid and chainID read the same info.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pgbarletta! Everything LGTM!

@orbeckst
Copy link
Member

ALong the lines of MDAnalysis/UserGuide#312 — please (1) raise an issue to document in the User Guide and then (2) do the documenting.

@orbeckst
Copy link
Member

@lilyminium I tentatively assigned you to merge because you reviewed but please feel free to punt.

@lilyminium lilyminium merged commit d41d42b into MDAnalysis:develop Sep 16, 2023
18 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in PDBQT to PDB conversion
4 participants