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

Fix charge reading from PDBQTParser #4283

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

pgbarletta
Copy link
Contributor

@pgbarletta pgbarletta commented Sep 6, 2023

Fixes #4282

Changes made in this Pull Request:

  • Columns 67 to 70 from PDBQT files are ignored instead of trying to parse them as atom charges.

PR Checklist

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

Developers certificate of origin


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

@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:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/6525284582/job/17717806569


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0b9bfd5) 93.40% compared to head (f015129) 93.37%.
Report is 11 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4283      +/-   ##
===========================================
- Coverage    93.40%   93.37%   -0.04%     
===========================================
  Files          170      184      +14     
  Lines        22256    23403    +1147     
  Branches      4071     4075       +4     
===========================================
+ Hits         20788    21852    +1064     
- Misses         951     1035      +84     
+ Partials       517      516       -1     
Files Coverage Δ
package/MDAnalysis/coordinates/PDBQT.py 83.50% <ø> (ø)
package/MDAnalysis/topology/PDBQTParser.py 100.00% <100.00%> (ø)

... and 18 files with indirect coverage changes

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

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.

thanks @pgbarletta this looks like a good catch. If you have time adding a test (or updating whatever pdbqt file we currently have) to include a remark in cols 67-70 would make sure this behaviour is tested

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Please add a test that fails without the fix and passes with it.

Please also include a link to the format document in the docs. The more we can say very clearly which format we are implementing the better.

Finally, add a versionchanged (take into account the "footnote" field in coulmns ....) because it's possible that the standard changed without us knowing and then we want to have a record of when we changed.

@pep8speaks
Copy link

pep8speaks commented Sep 20, 2023

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

Line 91:80: E501 line too long (80 > 79 characters)

Line 109:22: W291 trailing whitespace

Comment last updated at 2023-10-15 16:43:30 UTC

@pgbarletta pgbarletta requested a review from orbeckst September 21, 2023 00:40
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Thanks for your changes. All looking good (will just make a minor reformatting change).

I am very sorry, the review slipped through. As always, please feel free to ping me if there's no action in a few days.

package/MDAnalysis/topology/PDBQTParser.py Outdated Show resolved Hide resolved
@orbeckst orbeckst self-assigned this Oct 15, 2023
@orbeckst orbeckst merged commit 913ca7b into MDAnalysis:develop Oct 15, 2023
20 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.

PDBQTParser.parse() assigns wrong field to charges
5 participants