fix error message for XDR readers by orbeckst · Pull Request #4231 · MDAnalysis/mdanalysis

orbeckst · 2023-08-10T17:45:36Z

Fixes #

Changes made in this Pull Request:

error message for not being able to write lockfile did not insert the filename into message: fixed by using proper f string
changed all .format to modern f string in XDR.py
add explicit test for offsets_filename()
add additional pytest.warns() to tests to catch warnings (reduces warnings output and checks that all expected warnings are raised with proper messages)

PR Checklist

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

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

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

- error message for not being able to write lockfile did not insert the filename into message: fixed by using proper f string - changed all .format to modern f string in XDR.py - add explicit test for offsets_filename() - add additional pytest.warns() to tests to catch warnings (reduces warnings output and checks that all expected warnings are raised with proper messages)

orbeckst · 2023-08-10T17:47:31Z

FYI, this is what the (small) error looks like in practice

>>> u = mda.Universe("md-01.tpr", "md-01.part0001.xtc")
~/miniconda3/envs/mda25/lib/python3.10/site-packages/MDAnalysis/coordinates/XDR.py:203: UserWarning: Cannot write lock/offset file in same location as {self.filename}. Using slow offset calculation.
  warnings.warn(f"Cannot write lock/offset file in same location as "
~/miniconda3/envs/mda25/lib/python3.10/site-packages/MDAnalysis/coordinates/XDR.py:259: UserWarning: Couldn't save offsets because: [Errno 13] Permission denied: '.md-01.part0001.xtc_offsets.npz'
  warnings.warn("Couldn't save offsets because: {}".format(e))

Note how {self.filename} is not replaced.

github-actions · 2023-08-10T17:47:53Z

Linter Bot Results:

Hi @orbeckst! 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	⚠️ Possible failure

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/5828228895/job/15805580193

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

IAlibay

Couple of small things, otherwise lgtm

codecov · 2023-08-10T18:04:04Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d33cc3a) 93.62% compared to head (593aa9f) 93.62%.
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4231   +/-   ##
========================================
  Coverage    93.62%   93.62%           
========================================
  Files          193      193           
  Lines        25295    25304    +9     
  Branches      4063     4064    +1     
========================================
+ Hits         23683    23692    +9     
  Misses        1096     1096           
  Partials       516      516

Files Changed	Coverage Δ
package/MDAnalysis/coordinates/XDR.py	`97.72% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

IAlibay

cheers, lgtm!

orbeckst · 2023-08-10T18:19:28Z

+        assert saved_offsets == False

-    def test_nonexistent_offsets_file(self, traj):
+    def test_corrupted_offsets_file(self, traj):


btw, this shadowed the preceeding test; renaming increased our tests

orbeckst · 2023-08-10T18:57:03Z

+            assert len(record) >= len(warning_messages)
+            found_messages = [str(r.message) for r in record]
+            # find at least 2 occurences (>= instead of == for robustness)
+            assert sum([ref in msg for msg in found_messages


I looked through the other tests in the file and found the following approach that looks a bit cleaner where we stack pytest.warns:

with (pytest.warns(UserWarning, match="Couldn't save offsets") and pytest.warns(UserWarning, match="Cannot write")): self._reader(filename)

I'll rewrite to use that idiom.

…sis into fix-xdr-errormessages

- reduces warnings in test_xdr down to 2 (both related to del) - test for empty selection and missing elements warnings - rewriting more old-school parameter interpolation to f strings

orbeckst · 2023-08-10T19:13:43Z

Thanks for the quick review @IAlibay . I apologize, I tacked on a few little things that I couldn't let go when I looked at the code again.

Co-authored-by: Rocco Meli <r.meli@bluemail.ch>

…sis into fix-xdr-errormessages

orbeckst · 2023-08-11T16:23:45Z

One of the runners timed out so I restarted it.

orbeckst · 2023-08-11T16:27:58Z

@RMeli or @IAlibay could one of you please volunteer to captain the PR and eventually merge it? I can do it if you prefer but it's better style (in my opinion) to not be the one merging your own PR (unless it's urgent....).

I think it's good for the project when there's visibly someone in charge of a PR.

IAlibay · 2023-08-11T17:09:16Z

lgtm!

github-actions Bot added the Component-Readers label Aug 10, 2023

IAlibay requested changes Aug 10, 2023

View reviewed changes

Comment thread testsuite/MDAnalysisTests/coordinates/test_xdr.py

Comment thread testsuite/MDAnalysisTests/coordinates/test_xdr.py

Comment thread testsuite/MDAnalysisTests/coordinates/test_xdr.py Outdated

orbeckst and others added 2 commits August 10, 2023 11:17

more robust checking of multiple warnings

4f18afb

Apply suggestions from code review

9b0c1c9

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

IAlibay approved these changes Aug 10, 2023

View reviewed changes

orbeckst commented Aug 10, 2023

View reviewed changes

orbeckst added 3 commits August 10, 2023 12:00

simplified testing for multiple warnings

997e15c

Merge branch 'fix-xdr-errormessages' of github.com:MDAnalysis/mdanaly…

fd4fe34

…sis into fix-xdr-errormessages

more explicit warnings testing

4f8be63

- reduces warnings in test_xdr down to 2 (both related to del) - test for empty selection and missing elements warnings - rewriting more old-school parameter interpolation to f strings

IAlibay requested changes Aug 10, 2023

View reviewed changes

Comment thread testsuite/MDAnalysisTests/coordinates/test_xdr.py Outdated

RMeli reviewed Aug 10, 2023

View reviewed changes

Comment thread package/MDAnalysis/coordinates/XDR.py Outdated

Comment thread testsuite/MDAnalysisTests/coordinates/test_xdr.py Outdated

orbeckst and others added 3 commits August 10, 2023 19:16

only use f-strings where used

f995b31

Co-authored-by: Rocco Meli <r.meli@bluemail.ch>

ignore warnings not germane to the XDR tests

874d9ba

Merge branch 'fix-xdr-errormessages' of github.com:MDAnalysis/mdanaly…

593aa9f

…sis into fix-xdr-errormessages

IAlibay approved these changes Aug 11, 2023

View reviewed changes

IAlibay merged commit 6e80c28 into develop Aug 11, 2023

IAlibay deleted the fix-xdr-errormessages branch August 11, 2023 17:09

orbeckst assigned IAlibay Aug 11, 2023

IAlibay added the maintainability label Sep 21, 2023

Conversation

orbeckst commented Aug 10, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Developers certificate of origin

Uh oh!

orbeckst commented Aug 10, 2023

Uh oh!

github-actions Bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linter Bot Results:

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

orbeckst Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Aug 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst commented Aug 11, 2023

Uh oh!

orbeckst commented Aug 11, 2023

Uh oh!

IAlibay commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

orbeckst commented Aug 10, 2023 •

edited by github-actions Bot

Loading

github-actions Bot commented Aug 10, 2023 •

edited

Loading

codecov Bot commented Aug 10, 2023 •

edited

Loading