different atom number error message

[Yibo-Zhang]

Fixes #1998

Changes made in this Pull Request:

• move the 'check if atom number changed' above conversion part. It will give the clear error message when atom numbers are different.

PR Checklist

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

[richardjgowers]

Not sure why appveyor failed, ignore that. Can you check that there’s a test for a pdb file with varying atoms per frame? Should be in one of the test_pdb.py files

[Yibo-Zhang]

Hi @richardjgowers , pytest test_pdb.py, it did not show errors related to the varing atoms per frame.

[zemanj]

Error message needs to be improved and, as @richardjgowers already mentioned, tests are needed as well.

[zemanj]

The error message is still somewhat misleading. In fact, the number of atoms read is correct, and reading has nothing to do with the error. Also, the number of atoms read from the trajectory frame reported in the error message seems to be wrong (pos+1 instead of pos). I'd suggest changing the error to

...
        if pos != self.n_atoms:
            raise ValueError("Inconsistency in file '{}': The number of atoms "
                             "({}) in trajectory frame {} differs from the "
                             "number of atoms ({}) in the corresponding "
                             "topology.\nTrajectories with varying numbers of "
                             "atoms are currently not supported."
                             "".format(self.filename, pos, frame, self.n_atoms)

[codecov]

Codecov Report

Merging #2219 into develop will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2219      +/-   ##
===========================================
+ Coverage    89.57%   89.58%   +<.01%     
===========================================
  Files          158      158              
  Lines        19724    19687      -37     
  Branches      2780     2769      -11     
===========================================
- Hits         17668    17636      -32     
+ Misses        1460     1457       -3     
+ Partials       596      594       -2

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/PDB.py	90.54% <100%> (+0.6%)	⬆️
package/MDAnalysis/topology/PSFParser.py	85.21% <0%> (-2.61%)	⬇️
package/MDAnalysis/analysis/waterdynamics.py	81.13% <0%> (-1.56%)	⬇️
util.py	88.14% <0%> (-0.02%)	⬇️
package/MDAnalysis/core/groups.py	98.26% <0%> (ø)	⬆️
package/MDAnalysis/lib/util.py	88.46% <0%> (+0.13%)	⬆️
package/MDAnalysis/core/universe.py	94.98% <0%> (+0.55%)	⬆️
package/MDAnalysis/analysis/nuclinfo.py	92.25% <0%> (+2.11%)	⬆️

---

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 668e183...3d554a9. Read the comment docs.

[Yibo-Zhang]

Hi @zemanj . Thanks for your suggestion. I looked through the test_PDB file and not find the test I need. So I try to write one like

    def test_atom_not_match(self):
        from MDAnalysis.coordinates.PDB import PDBReader
        reader = PDBReader(PDB_singleframe)
        with pytest.raises(ValueError) as excinfo:
            reader._read_frame(0)
        assert 'Inconsistency in file' in str(excinfo.value)

However, I do not know how to make the ValueError happen. Also when I try to give PDBReader a n_atoms parameter, it report indexerror, is there a test file that contain different atom number, that I can use to test it. Or maybe I'm totally wrong about this thing.

[richardjgowers]

Hey @Yibo-Zhang , the problem here is you need to trigger the error with the file you use in the test. The file you're currently using has nothing wrong with it, so you never check the piece of code you've modified. Instead, if you give it a pdb file where the number of atoms changes between frames, you can test that you get the expected behaviour (ie a ValueError with a useful error message).

Inside test_pdb there's some examples of using StringIO to create special pdb files that trigger certain conditions, you should be able to make a varying-atom pdb file with the same method.

[Yibo-Zhang]

Hey @richardjgowers , I added the test for the changed part, and it works. Can you have a quick review. Thanks

[orbeckst]

Looks good to me.

@richardjgowers and @zemanj do you want to have a quick look?

[orbeckst]

@Yibo-Zhang please also

• add your name to AUTHORS
• add an entry to CHANGELOG (under Fixes)

[orbeckst]

@Yibo-Zhang if you want this to get merged soon please add yourself to AUTHORS and add an entry to CHANGELOG #2219 (comment)

@zemanj please check if your comments have been addressed, your approval is missing. Thanks.

[Yibo-Zhang]

hey @orbeckst I added that two parts, thanks. I'm so happy that I can add my name at authors list.

[zemanj]

Conflicts need to be resolved, otherwise this looks really good!

[orbeckst]

Congratulations @Yibo-Zhang , your first PR has been merged, thank you for your contribution. I hope more will follow!