Fixes PM issues (jlab and non-AnalysisBase) #2310
Conversation
There was a problem hiding this comment.
Many thanks, I am happy to get this in, including your cleanup/testing in helanal.
One minor request (see comment): replace the custom FinishTimeException with a simple ValueError. I don't think there's a need for introducing another exception, is there?
I'll wait for Travis to come back with tests passing. Feel free to ping me when I should look again.
| # finish occurs before begin time | ||
| msg = ("The input finish time ({0} ps) precedes the input begin " | ||
| "time ({1} ps)".format(finish, begin)) | ||
| raise FinishTimeException(msg) |
There was a problem hiding this comment.
While you're at it, could you please get rid of FinishTimeException and just raise a ValueError (because the user provided a wrong value). Makes more sense...
| with tmpdir.as_cwd(): | ||
| with pytest.raises(FinishTimeException): | ||
| MDAnalysis.analysis.helanal.helanal_trajectory( | ||
| u, selection="name CA", finish=5 | ||
| ) | ||
|
|
||
| with tmpdir.as_cwd(): | ||
| with pytest.raises(FinishTimeException): |
There was a problem hiding this comment.
Let's use a ValueError, more in line with standard Python usage
Codecov Report
@@ Coverage Diff @@
## develop #2310 +/- ##
===========================================
+ Coverage 89.59% 89.65% +0.06%
===========================================
Files 157 157
Lines 19525 19530 +5
Branches 2803 2802 -1
===========================================
+ Hits 17493 17510 +17
+ Misses 1435 1427 -8
+ Partials 597 593 -4
Continue to review full report at Codecov.
|
|
Thanks for the review @orbeckst FinishTimeException was just used because it was the existing custom exception that seems to have been historically used in the helix analysis code. As per your suggestion, I have now changed everything to ValueError in 05849eb. Since this was the only part of the MDAnalysis code that used this custom exception, I went ahead and removed it completely from |
|
Yes, thank you, excellent! |
|
Thanks @IAlibay , sorry, took me a while to come back to. |
Fixes #2084 and #2078 (partly)
Changes made in this Pull Request:
So nearly a year late on this one. Here are a couple of changes to some of the ProgressMeter calls that caused issues with Jupyter Lab (note that even without these changes the latest version of Jupyter Lab doesn't cause what was seen in ProgressMeter doesn't work in jupyter lab #2078, although the extra carriage returns aren't necessary). I realise that as per replace custom ProgressMeter with tqdm #928 (comment) ProgressMeter is probably going to give way to
tdqmbut I thought I might as well push these changes whilsttdqmisn't implemented.This PR also updates the ProgressMeter behaviour of some of the currently non-AnalysisBase methods (namely density_from_Universe, HydrogenBondAutoCorrel, and helanal). Again this is probably not necessary in the long run (considering for example update analysis.base.AnalysisBase to conform to API #1463 (comment)) if the aim is to add everything to AnalysisBase. However I thought it probably was worth adding it in as a temporary fix.
This PR also adds a couple of warnings & exceptions to helanal that weren't being checked for when providing the
beginandfinisharguments.This PR does not provide any ProgressMeter changes to hbond_analysis or wbridge_analysis since they are currently being migrated to AnalysisBase (Hbond analysis #2237 and Move the water bridge analysis to the new analysis class #2087).
PR Checklist