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.

Sign up for GitHub

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

Jump to bottom

Add formalcharge attribute and read/write them from PDB #3755

Merged
merged 9 commits into from
Jul 23, 2022
Merged

Add formalcharge attribute and read/write them from PDB #3755

merged 9 commits into from
Jul 23, 2022

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Jul 17, 2022 •
edited
Loading

Fixes #3418

Changes made in this Pull Request:

  • Adds the formalcharge attribute - note this isn't the pretties names, so I'm 100% open to suggestions for renaming this.
  • Adds the ability to read and write formal charges from PDB files.
  • Clean up docs for the XPDB parser.

PR Checklist

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

@pep8speaks
Copy link

pep8speaks commented Jul 17, 2022 •
edited
Loading

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

Line 344:80: E501 line too long (80 > 79 characters)
Line 345:80: E501 line too long (80 > 79 characters)
Line 361:80: E501 line too long (80 > 79 characters)
Line 362:80: E501 line too long (80 > 79 characters)
Line 363:80: E501 line too long (80 > 79 characters)

Comment last updated at 2022-07-23 09:36:35 UTC

IAlibay
IAlibay commented Jul 17, 2022
View reviewed changes
package/MDAnalysis/coordinates/PDB.py

outcharges = charges.astype(object)
outcharges[outcharges == 0] = '' # empty strings for no charge case
# using np.where is more efficient than looping in sparse cases
Copy link
Member Author

Choose a reason for hiding this comment

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

We're looking at ~ 4x speedup for sparse arrays (which this will always be)

@codecov
Copy link

codecov bot commented Jul 17, 2022 •
edited
Loading

Codecov Report

Merging #3755 (d9636a7) into develop (50b8fdb) will increase coverage by 0.00%.
The diff coverage is 97.72%.

@@           Coverage Diff            @@
##           develop    #3755   +/-   ##
========================================
  Coverage    94.30%   94.31%           
========================================
  Files          192      192           
  Lines        24708    24752   +44     
  Branches      3328     3338   +10     
========================================
+ Hits         23302    23345   +43     
- Misses        1358     1359    +1     
  Partials        48       48
Impacted Files Coverage Δ
package/MDAnalysis/core/groups.py 98.58% <ø> (ø)
package/MDAnalysis/topology/ExtendedPDBParser.py 100.00% <ø> (ø)
package/MDAnalysis/core/topologyattrs.py 96.58% <87.50%> (-0.07%) ⬇️
package/MDAnalysis/coordinates/PDB.py 95.09% <100.00%> (+0.23%) ⬆️
package/MDAnalysis/topology/PDBParser.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50b8fdb...d9636a7. Read the comment docs.

richardjgowers
richardjgowers reviewed Jul 17, 2022
View reviewed changes
Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

Does the selection work if there's a space between the sign and the value? Eg "formalcharge + 1"

@IAlibay
Copy link
Member Author

IAlibay commented Jul 17, 2022 •
edited
Loading

Does the selection work if there's a space between the sign and the value? Eg "formalcharge + 1"

That'll throw (edit: as I would expect it to) a SelectionError, interestingly slightly different types of errors for + 1 and - 1 (in the former case it's an inability to parse + whilst in the latter it's that - 1 is an invalid expression).

Ranges do work here (since that's what int selections default to), so technically formalcharge -1-1 will select all the atoms with formal charges ranging from -1 through to +1.

hmacdope
hmacdope requested changes Jul 23, 2022
View reviewed changes
Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Looking good, just a few queries. :)

package/MDAnalysis/coordinates/PDB.py Outdated Show resolved Hide resolved
package/MDAnalysis/topology/PDBParser.py Outdated Show resolved Hide resolved
package/MDAnalysis/topology/PDBParser.py Outdated Show resolved Hide resolved
package/MDAnalysis/topology/PDBParser.py Outdated
else:
formalcharges[i] = 0
attrs.append(
FormalCharges(np.array(formalcharges, dtype=np.int32)))
Copy link
Member

Choose a reason for hiding this comment

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

See above question about int vs int32

Copy link
Member Author

Choose a reason for hiding this comment

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

Switching to int here since the topologyattr code will just re-type to this anyways unless dtype is set to None.

That being said, charges really shouldn't exceed the bounds of int8, so one wonders how wasteful this is... (can't remember if int32/int64 still has the weird behaviour of ending up faster than int8/int16 in some weird cases)

testsuite/MDAnalysisTests/topology/test_pdb.py
assert_equal(u.atoms.formalcharges, formal_charges)


PDB_charges_nosign = """\
Copy link
Member

Choose a reason for hiding this comment

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

Could possibly be a fixture but i guess its not recycled, up to you :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to skip on that, the advantages here are limited, especially given we use declared PDB strings like this everywhere else here.

@IAlibay IAlibay requested a review from hmacdope July 23, 2022 08:46
hmacdope
hmacdope approved these changes Jul 23, 2022
View reviewed changes
Copy link
Member

@hmacdope hmacdope left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for addressing comments @IAlibay looks good to roll. You can merge when ready.

@IAlibay
Copy link
Member Author

IAlibay commented Jul 23, 2022

oops I think I got my subtypes the wrong way around

@IAlibay IAlibay merged commit d5c7cb9 into develop Jul 23, 2022
@IAlibay IAlibay deleted the formalcharge-pdb branch July 23, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardjgowers richardjgowers richardjgowers left review comments

@hmacdope hmacdope hmacdope approved these changes

Assignees
No one assigned
Labels
Component-Core Component-Readers Component-Topology enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PDBParser does not save charges.
4 participants
@IAlibay @pep8speaks @richardjgowers @hmacdope