Skip to content

XYZ elements#2456

Merged
orbeckst merged 30 commits intoMDAnalysis:developfrom
RMeli:fixes/xyzelement
Jan 19, 2020
Merged

XYZ elements#2456
orbeckst merged 30 commits intoMDAnalysis:developfrom
RMeli:fixes/xyzelement

Conversation

@RMeli
Copy link
Member

@RMeli RMeli commented Jan 17, 2020

Fixes #2420 and #2421

Changes made in this Pull Request:

  • XYZParser adds elements attribute
  • XYZWriter tries to use the atoms.elements attribute (fallback to atoms.names)

PR Checklist

  • Tests
  • CHANGELOG updated
  • Issue raised/referenced

# Guessing time
atomtypes = guessers.guess_types(names)
masses = guessers.guess_masses(atomtypes)
masses = guessers.guess_masses(names)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I assume that names contain element symbols (XYZ file format). As discusses in #2420 this might not always be the case. Should I make this more flexible?

Copy link
Member

Choose a reason for hiding this comment

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

I think (hope) guess_masses is smart enough to skip a bad value and print a warning?

@RMeli
Copy link
Member Author

RMeli commented Jan 17, 2020

@richardjgowers tests for the writer are still missing...

Any suggestions on how to best write them?

@richardjgowers
Copy link
Member

@RMeli something like...

import io
import MDAnalysis as mda

u = mda.Universe.empty(n_atoms=5)

u.add_TopologyAttr('elements', values=['Te', 'S', 'Ti', 'N', 'Ga'])

ns = mda.lib.util.NamedStream(io.StringIO(), 'file.xyz')

with mda.Writer(ns) as w:
    w.write(u)

names = ''.join(l.split()[0] for l in ns.readlines())

assert names[:-1].lower() == 'testing'

@RMeli
Copy link
Member Author

RMeli commented Jan 17, 2020

@richardjgowers Thanks for the funny suggestion. I meant to ask how to best incorporate them in the existing suite... I put the tests in TestXYZWriterNames and the they try writing with both elements and names attributes.

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #2456 into develop will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2456      +/-   ##
===========================================
+ Coverage    89.99%   90.01%   +0.01%     
===========================================
  Files          177      177              
  Lines        22439    22233     -206     
  Branches      2914     2908       -6     
===========================================
- Hits         20193    20012     -181     
+ Misses        1637     1619      -18     
+ Partials       609      602       -7
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/XYZ.py 87.97% <ø> (-0.16%) ⬇️
package/MDAnalysis/topology/XYZParser.py 100% <ø> (ø) ⬆️
package/MDAnalysis/visualization/streamlines.py 88.18% <ø> (ø) ⬆️
package/MDAnalysis/analysis/waterdynamics.py 82.68% <ø> (+0.17%) ⬆️
package/MDAnalysis/visualization/streamlines_3D.py 94.08% <ø> (ø) ⬆️
package/MDAnalysis/coordinates/memory.py 96.05% <0%> (ø) ⬆️
package/MDAnalysis/topology/TXYZParser.py 100% <100%> (ø) ⬆️
package/MDAnalysis/coordinates/TXYZ.py 90.1% <100%> (-0.22%) ⬇️
package/MDAnalysis/lib/util.py 88.47% <100%> (+1.08%) ⬆️
package/MDAnalysis/auxiliary/base.py 86.11% <50%> (-0.86%) ⬇️
... and 49 more

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 11c5bab...3fca537. Read the comment docs.

@RMeli RMeli changed the title [WIP] XYZ elements XYZ elements Jan 19, 2020
@orbeckst
Copy link
Member

Given that @richardjgowers approved and I checked the codecov is nothing to worry about (the low diff coverage comes from changes in indentations that don't even show up in these files – weird) so I am merging,

@orbeckst orbeckst merged commit 3b7e86e into MDAnalysis:develop Jan 19, 2020
@RMeli RMeli deleted the fixes/xyzelement branch January 21, 2020 10:15
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.

XYZ parser guesses the elements

4 participants