TRZReader check for n_atoms by yuxuanzhuang · Pull Request #2820 · MDAnalysis/mdanalysis

yuxuanzhuang · 2020-07-03T08:59:37Z

Fixes #2817

Changes made in this Pull Request:

check for dt to raise ValueError if n_atoms is incompatible with the trajectory.

PR Checklist

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

update to HEAD

…into develop

codecov · 2020-07-03T11:03:37Z

Codecov Report

Merging #2820 into develop will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2820      +/-   ##
===========================================
+ Coverage    92.19%   92.22%   +0.02%     
===========================================
  Files          184      184              
  Lines        24072    24141      +69     
  Branches      3122     3123       +1     
===========================================
+ Hits         22194    22263      +69     
  Misses        1813     1813              
  Partials        65       65

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/TRZ.py	`88.51% <100.00%> (+0.08%)`	⬆️
util.py	`88.10% <0.00%> (+0.08%)`	⬆️
coordinates/base.py	`94.82% <0.00%> (+0.28%)`	⬆️
auxiliary/base.py	`91.31% <0.00%> (+0.56%)`	⬆️

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 17969df...a8275ae. Read the comment docs.

orbeckst

Can we check this in a more explicit manner?

orbeckst · 2020-07-03T23:11:49Z

package/CHANGELOG

  * TOPParser no longer guesses elements when missing atomic number records
    (Issues #2449, #2651)
  * Testsuite does not any more matplotlib.use('agg') (#2191)
+  * TRZReader now checks `n_atoms` provided right during initilization. 


Suggested change

* TRZReader now checks `n_atoms` provided right during initilization.

* TRZReader now checks `n_atoms` during initialization (PR #2820)

orbeckst · 2020-07-03T23:13:00Z

testsuite/MDAnalysisTests/coordinates/test_trz.py


+    def test_get_wrong_n_atoms(self):
+        with pytest.raises(ValueError, match=r"Supplied n_atoms *"):
+            mda.Universe(TRZ, n_atoms = 8080)


Suggested change

mda.Universe(TRZ, n_atoms = 8080)

mda.Universe(TRZ, n_atoms=8080)

orbeckst · 2020-07-03T23:20:46Z

package/MDAnalysis/coordinates/TRZ.py

+        try:
+            self._get_dt()
+        except OSError:
+            raise ValueError("Supplied n_atoms {} is incompatible "
+                             "with provided trajectory file. "
+                             "Maybe `topology` is wrong?".format(self.n_atoms))
+
+


It is not clear why _get_dt() will fail with the wrong n_atom. What's the stack trace and where does the ValueError come from? (I assume this relies on being able to do self.next(), which actually fills the ts.... but then why does self._read_next_timestep() on the preceding line work? 🤷 )

This might work and fix an error but I think we should rather check sooner and in a more transparent manner if at all possible.

_read_next_timestep (used by __next__) only fills the ts by reading a chunk from the TRZ file using np.fromfile with the provided frame_contents (defined by n_atoms) no matter it is right or wrong.

read_frame, and other related functions (e.g. _get_dt will go next and read frame back), on the other hand, will use self._seek. With an abnormal trajectory.next().frame, This will always reach an error.

yuxuanzhuang · 2020-07-04T13:49:15Z

Given my current knowledge on TRZ files (which is none), I cannot really come up with an ideal checking method. Maybe check if self.next() return a nonphysical frame? Or what may happen if TRZ only has one frame.

Besides, I checked there are still other magic n_atoms numbers that will pass the "dt" test, may leading to false positive cases.

Other people with more knowledge are welcome to step in.

IAlibay · 2020-07-04T14:12:34Z

Given my current knowledge on TRZ files (which is none), I cannot really come up with an ideal checking method. Maybe check if self.next() return a nonphysical frame? Or what may happen if TRZ only has one frame.

Besides, I checked there are still other magic n_atoms numbers that will pass the "dt" test, may leading to false positive cases.

Other people with more knowledge are welcome to step in.

I'm also not super well read on the TRZ format, but just looking at the code, is there any chance we could evaluate the return value of data['natoms']?

pep8speaks · 2020-07-06T11:45:00Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-07 14:10:06 UTC

yuxuanzhuang · 2020-07-06T11:51:49Z

Looks good? Also spot that TRZWriter does not write n_atoms to the file. Maybe a bug or intentionally?

richardjgowers

Looks awesome, thanks @yuxuanzhuang

IAlibay

@yuxuanzhuang could you just check that the docs build properly on this one? I think the versionchanged is going to look weird as-is.

package/MDAnalysis/coordinates/TRZ.py

IAlibay · 2020-07-06T18:47:32Z

testsuite/MDAnalysisTests/coordinates/test_trz.py

            assert_equal(W.n_atoms, 100)

+    def test_get_wrong_n_atoms(self):
+        with pytest.raises(ValueError, match=r"Supplied n_atoms *"):


note, I dont think you need to have the "*" in match calls, it essentially does a string in string type of comparison.

yuxuanzhuang · 2020-07-06T21:55:25Z

I just want to discuss the necessity of raising an error, i.e. the TRZ file is still valid if the natoms written in the file is wrong (e.g. 0), which is case for the previous TRZWriter. Maybe a warning will suffice? Or we still need to find something else for validation.

package/MDAnalysis/coordinates/TRZ.py

orbeckst · 2020-07-06T23:35:50Z

I just want to discuss the necessity of raising an error, i.e. the TRZ file is still valid if the natoms written in the file is wrong (e.g. 0), which is case for the previous TRZWriter. Maybe a warning will suffice? Or we still need to find something else for validation.

Let's raise an exception.

Did you fix the writer? If so, add a note to Fixes in the CHANGELOG, please.

IAlibay

Just a text clarification and an extra test and we're good I think.

IAlibay · 2020-07-07T09:25:13Z

package/CHANGELOG

  * Testsuite does not any more matplotlib.use('agg') (#2191)
  * In ChainReader, read_frame does not trigger change of iterating position. 
    (Issue #2723, PR #2815)
+  * TRZReader now checks `n_atoms` during initilization. (PR #2820)


I might be missing something obvious, but the check is happening in _read_next_timestep right? So technically it's not limited to initialization but every step. It might be more accurate here to just say "checks n_atoms on reading"?

Could you also add the issue number here?

package/MDAnalysis/coordinates/TRZ.py

IAlibay

lgtm :)

* fix #2817 * TRZReader checks that TRZ file contains the same number of atoms as the topology or n_atoms and fails otherwise * TRZWriter correctly writes n_atoms to file (previously wrote 0!) * add test * add docs * update CHANGELOG (cherry picked from commit c8abbee)

* fix MDAnalysis#2817 * TRZReader checks that TRZ file contains the same number of atoms as the topology or n_atoms and fails otherwise * TRZWriter correctly writes n_atoms to file (previously wrote 0!) * add test * add docs * update CHANGELOG

yuxuanzhuang added 12 commits May 20, 2020 10:32

Merge pull request #1 from MDAnalysis/develop

931d9b5

update to HEAD

Merge remote-tracking branch 'mda_origin/develop' into develop

d69ce98

enable travis temporarily

1453fb5

Merge remote-tracking branch 'mda_origin/develop' into develop

92a14a8

Merge branch 'develop' of https://github.com/yuxuanzhuang/mdanalysis …

950cfe3

…into develop

restore travis

8a923fe

Merge remote-tracking branch 'mda_origin/develop' into develop

9fe22a9

check for n_atoms for trz

953507f

changelog

a75d27e

test

84c0c33

blank

f2bf331

Merge branch 'develop' into trzreader_check

49d5626

orbeckst requested changes Jul 3, 2020

View reviewed changes

yuxuanzhuang added 2 commits July 6, 2020 13:35

check data.natoms

5977404

add n_atoms to writer

7178736

yuxuanzhuang added 2 commits July 6, 2020 13:46

Merge branch 'develop' into trzreader_check

d0c2e2c

pep8

1f086ea

richardjgowers approved these changes Jul 6, 2020

View reviewed changes

IAlibay requested changes Jul 6, 2020

View reviewed changes

orbeckst reviewed Jul 6, 2020

View reviewed changes

package/MDAnalysis/coordinates/TRZ.py Outdated Show resolved Hide resolved

orbeckst reviewed Jul 6, 2020

View reviewed changes

package/MDAnalysis/coordinates/TRZ.py Outdated Show resolved Hide resolved

orbeckst assigned orbeckst and richardjgowers Jul 6, 2020

orbeckst assigned IAlibay Jul 6, 2020

orbeckst added this to the 1.0.x milestone Jul 6, 2020

doc

a8a0180

IAlibay requested changes Jul 7, 2020

View reviewed changes

prec doc

a8275ae

IAlibay approved these changes Jul 7, 2020

View reviewed changes

orbeckst approved these changes Jul 7, 2020

View reviewed changes

orbeckst merged commit c8abbee into MDAnalysis:develop Jul 7, 2020

fiona-naughton added defect Component-Readers labels Sep 25, 2023

	* TRZReader now checks `n_atoms` provided right during initilization.
	* TRZReader now checks `n_atoms` during initialization (PR #2820)

	mda.Universe(TRZ, n_atoms = 8080)
	mda.Universe(TRZ, n_atoms=8080)

Conversation

yuxuanzhuang commented Jul 3, 2020

PR Checklist

Uh oh!

codecov bot commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

orbeckst Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

orbeckst Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

yuxuanzhuang Jul 4, 2020

Choose a reason for hiding this comment

Uh oh!

yuxuanzhuang commented Jul 4, 2020

Uh oh!

IAlibay commented Jul 4, 2020

Uh oh!

pep8speaks commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-07-07 14:10:06 UTC

Uh oh!

yuxuanzhuang commented Jul 6, 2020

Uh oh!

richardjgowers left a comment

Choose a reason for hiding this comment

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

yuxuanzhuang commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst commented Jul 6, 2020

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

IAlibay Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

IAlibay Jul 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jul 3, 2020 •

edited

Loading

pep8speaks commented Jul 6, 2020 •

edited

Loading

yuxuanzhuang commented Jul 6, 2020 •

edited

Loading