Skip to content

Ensure that GRO reader treats lack of unit cells consistently#3380

Merged
IAlibay merged 3 commits intodevelopfrom
issue-3305
Aug 13, 2021
Merged

Ensure that GRO reader treats lack of unit cells consistently#3380
IAlibay merged 3 commits intodevelopfrom
issue-3305

Conversation

@IAlibay
Copy link
Member

@IAlibay IAlibay commented Aug 1, 2021

Fixes #3305

Changes made in this Pull Request:

  • Add warning on reading / writing GRO files with empty box
  • Remove support for attempting to read GRO files with unit cells with neither 3 of 9 entries

PR Checklist

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

@IAlibay
Copy link
Member Author

IAlibay commented Aug 1, 2021

@lilyminium @mnmelo @jbarnoud I know you have many views on this, please do review.

@IAlibay IAlibay added this to the 2.0 milestone Aug 1, 2021
@codecov
Copy link

codecov bot commented Aug 1, 2021

Codecov Report

Merging #3380 (0d1d234) into develop (6531904) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3380      +/-   ##
===========================================
+ Coverage    93.62%   93.64%   +0.02%     
===========================================
  Files          176      176              
  Lines        22839    22839              
  Branches      3225     3225              
===========================================
+ Hits         21382    21387       +5     
+ Misses        1406     1401       -5     
  Partials        51       51              
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/GRO.py 96.64% <100.00%> (+2.85%) ⬆️
package/MDAnalysis/coordinates/PQR.py 95.23% <0.00%> (+0.91%) ⬆️

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 6531904...0d1d234. Read the comment docs.

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.

My brain hasn't really woken up yet, but I found those negatives hard to parse. Maybe telling the user what boxes are allowed is easier?

Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
@IAlibay IAlibay requested a review from lilyminium August 12, 2021 16:38
@lilyminium
Copy link
Member

LGTM thank you @IAlibay, I'll let you decide if you want to wait on further reviews or merge

@IAlibay IAlibay merged commit aba8b7d into develop Aug 13, 2021
@IAlibay IAlibay deleted the issue-3305 branch August 13, 2021 13:01
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.

GRO reader / writer, handle missing / incomplete unitcell data

2 participants