developing back end run methods #216

cadeduckworth · 2022-09-15T17:30:07Z

separating functionality for use with _single_universe without iterating frames
retaining functionality for use with _single_frame

* separating functionality for use with _single_universe without iterating frames * retaining functionality for use with _single_frame

* try/except pattern condensed to one run method * single run method implemented for _single_universe and _single_frame * functionality of original run method retained, while removing frame iteration for _single_universe usage * progress bar still active when using _single_universe, but no frame iteration occurs

orbeckst

Please address comments and add tests + CHANGELOG.

mdpow/analysis/ensemble.py

orbeckst · 2022-10-20T18:11:43Z

Update docs https://github.com/Becksteinlab/MDPOW/tree/develop/doc/sphinx/source/analysis : write how users are supposed to create EnsembleAnalysisClasses.

Develop a simple example that could be used as a tutorial or example in the docs.

This example can be the basis for a test.

removed raise added docstrings to describe use of NotImplementedError responded with reason for removing _conclude_universe() next step: updating html documentation following successful testing

fixed expected indent block error removed raise removed _conclude_universe()

…ntedError for _single_frame() and _single_universe() so try/except pattern in run method works properly

…nto ensemble_run_update

codecov · 2022-10-28T23:26:47Z

Codecov Report

Merging #216 (8870e39) into develop (cc104f0) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #216      +/-   ##
===========================================
+ Coverage    78.90%   79.15%   +0.25%     
===========================================
  Files           11       12       +1     
  Lines         1692     1713      +21     
  Branches       269      269              
===========================================
+ Hits          1335     1356      +21     
  Misses         274      274              
  Partials        83       83

Impacted Files	Coverage Δ
mdpow/analysis/dihedral.py	`100.00% <100.00%> (ø)`
mdpow/analysis/ensemble.py	`97.18% <100.00%> (+0.02%)`	⬆️
mdpow/__init__.py	`100.00% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…n method, mdpow/tests/test_run_except.py

…achieve an ordered selection format

…h assert atom group order

… pytest locally * selection string format updated, final, correct, and consistent with original intended selection order * updated values calculated for circmean, circvar for use in test_results_recursive2() * changed order for arguments in assert_almost_equal() for use in test_results_recursive2() to align with format in numpy.testing docs, (actual, desired) for readability, does not affect accuracy or testing purposes

…ralAnalysis to test_dihedral.py

…' into ensemble_run_update test merge

orbeckst · 2022-11-14T19:14:14Z

mdpow/analysis/ensemble.py


    def _single_frame(self):
        """Calculate data from a single frame of trajectory

        Called on each frame for universes in the Ensemble.
        """
-        pass  # pragma: no cover
+        raise NotImplementedError


Add test that checks that this NotImplementedError is raised. If necessary by calling the method explicitly

def test_single_frame_raises_NotImplementedError(): ... ea = EnsembleAnalysis(...) with pytest.raises(NotImplementedError): ea._single_frame()

https://github.com/Becksteinlab/MDPOW/blob/ensemble_run_update/mdpow/tests/test_dihedral.py

file does not exist in pull request because it was deleted and moved to another pull request, so I just linked the source from the branch

function is at end of module

this is the explicit call of _single_universe() to test NotImplementedError is raised when not defined, but specifically moved for use in DihedralAnalysis as instructed

I am going to add explicit tests for raising NotImplementedError for _single_frame() and _single_universe() similar to pattern in your comment above, placed in test_ensemble.py, because test_run.py is for the partition coefficient calculation and test_ensemble.py deals with EnsembleAnalysis class where this run method that calls single_frame/uni is defined

will be in this pull request

Look into https://github.com/Becksteinlab/MDPOW/pull/216/files and you see the annotation by codecov that says that the line was not covered by tests.

Also check the codecov report in the checks and ultimately https://app.codecov.io/gh/Becksteinlab/MDPOW/pull/216

orbeckst

There are some edits that are now being fixed in PR #218 so don't make the changes here and wait for #218 to be merged.

See other inline comments but overall looking good!

EDIT: add entry to CHANGES

orbeckst · 2022-11-14T19:16:49Z

mdpow/analysis/ensemble.py

+
+        NotImplementedError will detect whether _single_universe or _single_frame
+        should be implemented, based on which is defined in the EnsembleAnalysisClass.
+        Only one of the two aforementioned functions should be defined for the respective
+        analysis class. For verbose functionality, the analysis will currently show two
+        iteration bars, where only one of which will actually be iterated, while the other
+        will load to completion instantaneously, showing the system that is being worked on.


These implementation details are developer documentation and do not need to be in run() — users read these docs and will be confused. Put it into the part of the docs that talks about EnsembleAnalysis and how to write your own. Add a section to https://mdpow.readthedocs.io/en/latest/analysis/ensemble_analysis.html

Do I need to create a Read the Docs account and link my GitHub account and repository to make these changes as indicated in the Sphinx documentation, or do I make the changes to the text files (i.e. ensemble_analysis.txt) and push the changes straight to GitHub?

You don't need an account. The CI is already using RTD.

You only need to edit the docs here, namely ensemble_analysis.txt.

The CI puts an updated view of the docs for the PR at https://mdpow--216.org.readthedocs.build/en/216/

Thank you! I think everything merged and pushed correctly, and all tests passed. I do need to modify the docs slightly after following the link above, but will wait until your review to make all necessary documentation changes in one commit.

mdpow/tests/test_dihedral.py

* test_ensemble.py ** test_ensemble_analysis_run() ** testing NotImplementedError is raised for: *** _single_frame() *** _single_universe() * passes locally

…ted-dihedral-analysis updating with changes for backend and testing from PR #216 ensemble_run_update

…r EnsembleAnalysis run methods to _single_frame and _single_universe, including new tests for NotImplementedError usage

orbeckst

Please see comments for guidance on doc writing.

Also need tests.

orbeckst · 2022-12-08T01:09:47Z

CHANGES

@@ -5,7 +5,26 @@ CHANGES for MDPOW
 Add summary of changes for each release. Use ISO 8061 dates. Reference
 GitHub issues numbers and PR numbers.

+2022-12-07    0.8.1


change to 0.9.0 — we are making API changes so under semantic versioning this cannot be a patch release.

turn the date into 2022-??-?? — we don't know a release date yet

In fact: add everything to the 0.8.1 entry below and change that 0.8.1 to 0.9.0

orbeckst · 2022-12-08T01:10:44Z

CHANGES

+* defined new test for updated ensemble.EnsembleAnalysis.run() method (#216):
+  - new test, test_ensemble_analysis_run(), uses TestAnalysis class, 
+    is in test_ensemble.py
+  - new test checks that NotImplementedError is raised when _single_frame 
+    or _single_universe are not defined, to implement correct method


remove — CHANGES only lists what's important to users

orbeckst · 2022-12-08T01:11:41Z

CHANGES

+* source and html docs updated for EnsembleAnalysis
+  to reflect changes and describe proper usage


remove — not relevant to users (this goes in the commit message)

CHANGES does not just collect commit messages, it summarizes changes at a high level with a view towards what a user of the code needs to know when they upgrade

orbeckst · 2022-12-08T01:17:52Z

doc/sphinx/source/analysis/ensemble_analysis.txt

 systems. This allows for users to easily run analyses on MDPOW simulations.

+NotImplementedError will detect whether _single_universe or _single_frame


Use sphinx reST markup

Suggested change

NotImplementedError will detect whether _single_universe or _single_frame

:exc:`NotImplementedError` will detect whether :meth:`_single_universe` or :meth:`_single_frame`

For specific Python directives and roles see https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#the-python-domain (leave out a leading :py: because that's the default for us). See also https://www.sphinx-doc.org/en/master/usage/restructuredtext/index.html for more on reST.

To actually make the markup link to the doc, use something like

:meth:`~EnsembleAnalysis._single_universe`

The tilde will make it appear as only _single_universe.

orbeckst · 2022-12-08T01:21:41Z

doc/sphinx/source/analysis/ensemble_analysis.txt

 systems. This allows for users to easily run analyses on MDPOW simulations.

+NotImplementedError will detect whether _single_universe or _single_frame
+should be implemented, based on which is defined in the EnsembleAnalysisClass.


mark up

:class:`EnsembleAnalysis`

Note that this works (or should work) because you're currently documenting the mdpow.analysis.ensemble module.

But if it doesn't work (i.e., you go to the HTML docs on RTD and clicking on EnsembleAnalysis does NOT lead you to the full docs) then write it as

:class:`mdpow.analysis.ensemble.EnsembleAnalysis`

if you want to see the full "path" or

:class:`~mdpow.analysis.ensemble.EnsembleAnalysis`

if you only want the class name.

orbeckst · 2022-12-08T01:25:13Z

mdpow/analysis/ensemble.py

+           Run on each universe in the ensemble during when
+           self.run in called.
+
+           NotImplementedError will detect whether _single_universe


mark-up NotImplementedError and _single_universe and _single_frame

orbeckst · 2022-12-08T01:26:25Z

mdpow/analysis/ensemble.py

+
+           NotImplementedError will detect whether _single_universe
+           or _single_frame should be implemented, based on which 
+           is defined in the EnsembleAnalysisClass.


is defined in the :class:`EnsembleAnalysis` class.

Only use the proper identifiers, don't make any up by adding "Class" to the end.

orbeckst · 2022-12-08T01:26:40Z

mdpow/analysis/ensemble.py

+           NotImplementedError will detect whether _single_universe
+           or _single_frame should be implemented, based on which 
+           is defined in the EnsembleAnalysisClass.


mark up etc

orbeckst · 2022-12-08T01:27:27Z

mdpow/analysis/ensemble.py

+
+           NotImplementedError will detect whether _single_universe
+           or _single_frame should be implemented, based on which 
+           is defined in the EnsembleAnalysisClass.
        """
        raise NotImplementedError


Your tests do not check that the base class raises NotImplementedError.

orbeckst · 2022-12-08T01:28:17Z

mdpow/analysis/ensemble.py

@@ -509,15 +517,8 @@ def run(self, start=None, stop=None, step=None):
        on each frame in the system.

        First iterates through keys of ensemble, then runs _setup_system


add markup

:meth:`_setup_system`

cadeduckworth · 2022-12-08T02:01:12Z

mdpow/tests/test_ensemble.py

+                with pytest.raises(NotImplementedError) as excinfo:
+                    TestAnalysis._single_universe(self)
+                assert excinfo.type == 'NotImplementedError'
+


#216 #216 (comment)

@orbeckst Can you provide more detail for the linked comment? This test might not have shown up with the recent commit, but if it did, can you elaborate on how it is not correctly testing the base class?

Codecov indicates that these lines were not tested. Codecov is not always right but it's something to look into carefully. Come up with a way to convince yourself (and me) that your tests really test the code that you think they do. Perhaps temporarily change the raise NotImplementError to ValueError and then your test should fail. Report back on what you did.

I changed exception/error types and nothing changed. If the test and class definitions were nested correctly, pytest would have also picked up that
assert excinfo.type == should have been assert excinfo.type is, so basically nothing was happening. There is a way to match directly to string but I did it incorrectly and this way is simpler.

I split the single test into two separate, one for _singe_frame and one for _single_universe. Each has its own TestAnalysis class definition, where it shows that the run method not being tested is defined, so the undefined run method is explicitly tested for raising NotImplementedError, for each case.

There is probably a way to consolidate the two tests back into one, but it is very clear what is being done this way. It still seems there is a simpler method. I will review again in the morning and make changes requested for documentation so this can be merged if everything is correct otherwise!

…ne test for _single_universe, tested with other error types to confirm coverage

orbeckst · 2022-12-08T20:37:28Z

Now coverage of the patch is 100%, that's good. Please ping me when you have addressed the remaining comments (docs, CHANGES, etc), @cadeduckworth . Thanks!

…alysis

…rsion information in CHANGES

orbeckst

Looking good, just minor comments.

orbeckst · 2022-12-09T01:22:30Z

CHANGES

+  and use _single_frame OR _single_universe (#216)
+* _prepare_universe and _conclude_universe removed from 
+  EnsembleAnalysis.run() method, no longer needed (per comments, #199)
+


remove empty line

Suggested change

orbeckst · 2022-12-09T01:26:08Z

mdpow/analysis/dihedral.py

+                    msg = '''Dihedral calculations require AtomGroups with
                              only 4 atoms, %s selected''' % len(group)


reformat this message so that it does not contain newline/space

Suggested change

msg = '''Dihedral calculations require AtomGroups with

only 4 atoms, %s selected''' % len(group)

msg = ("Dihedral calculations require AtomGroups with "

f"only 4 atoms, {len(group)} selected")

And we can use f-strings.

orbeckst · 2022-12-09T01:28:01Z

mdpow/analysis/ensemble.py

+        """Calculations on a single :class:`MDAnalysis.Universe <MDAnalysis.core.groups.universe.Universe>` object.
+
+           Run on each :class:`MDAnalysis.Universe <MDAnalysis.core.groups.universe.Universe>` 
+           in the :class:`~mdpow.analysis.ensemble.Ensemble` 
+           during when :meth:`run` in called.
+
+           :exc:`NotImplementedError` will detect whether 
+           :meth:`~EnsembleAnalysis._single_universe`
+           or :meth:`~EnsembleAnalysis._single_frame` 
+           should be implemented, based on which is defined 
+           in the :class:`~mdpow.analysis.ensemble.EnsembleAnalysis`.


Can you format it so that it fits into 79 character wide lines?

The first line can be broken over multiple lines, there should just be an empty line to separate the summary/header line from the synopsis.

orbeckst · 2022-12-09T01:41:46Z

mdpow/tests/test_dihedral.py

+    def test_single_universe(self):
+        dh = self.Ens.select_atoms('name C4', 'name C17', 'name S2', 'name N3')
+        with pytest.raises(NotImplementedError):
+            DihedralAnalysis([dh])._single_universe()
+


This test is not strictly necessary but we can leave it in, as a check that the DihedralAnalysis does not do something weird to _single_universe().

orbeckst · 2022-12-12T20:33:40Z

Congratulations @cadeduckworth , PR is merged! 🚀

developing back end run methods

e71456d

* separating functionality for use with _single_universe without iterating frames * retaining functionality for use with _single_frame

cadeduckworth marked this pull request as draft September 15, 2022 17:30

orbeckst mentioned this pull request Oct 20, 2022

Automated Dihedral Analysis #217

Merged

orbeckst requested changes Oct 20, 2022

View reviewed changes

mdpow/analysis/ensemble.py Outdated Show resolved Hide resolved

mdpow/analysis/ensemble.py Show resolved Hide resolved

cadeduckworth and others added 9 commits October 21, 2022 20:21

Addressed change requests by Oliver

7bcdf49

removed raise added docstrings to describe use of NotImplementedError responded with reason for removing _conclude_universe() next step: updating html documentation following successful testing

fixed indentation error for _single_frame() block

a87cb11

reverting previous tab issue, necessary for normal function

7edde0d

fixed errors previously reverted for testing

48016c5

fixed expected indent block error removed raise removed _conclude_universe()

Merge branch 'develop' into ensemble_run_update

59c819c

doc string format/syntax issue

fb58f27

checkout from alt repository

1f7f2d1

changed base class function definitions from pass to raise NotImpleme…

179603f

…ntedError for _single_frame() and _single_universe() so try/except pattern in run method works properly

Merge branch 'ensemble_run_update' of github.com:Becksteinlab/MDPOW i…

46d3cf2

…nto ensemble_run_update

cadeduckworth marked this pull request as ready for review October 28, 2022 23:26

cadeduckworth added 8 commits November 1, 2022 21:00

adding preliminary test for try-except pattern addition to backend ru…

f47c806

…n method, mdpow/tests/test_run_except.py

removed # pragma: no cover from _single_frame() and _single_universe()

5e02322

changed dihedral atom group selection strings in test_dihedral.py to …

2fc9135

…achieve an ordered selection format

changed dihedral atom group selection order in test_dataframe to matc…

37cd1d7

…h assert atom group order

relocating test to external branch, 214_dihedral issue

a0de2d9

moved raise NotImplementedError test for _single_universe() for Dihed…

273c1af

…ralAnalysis to test_dihedral.py

Merge remote-tracking branch 'origin/214_dihedralanalysis-test-issues…

8212d11

…' into ensemble_run_update test merge

orbeckst reviewed Nov 14, 2022

View reviewed changes

orbeckst requested changes Nov 14, 2022

View reviewed changes

cadeduckworth added 2 commits November 17, 2022 03:38

added explicit backend run method tests for EnsembleAnalysis

653210e

* test_ensemble.py ** test_ensemble_analysis_run() ** testing NotImplementedError is raised for: *** _single_frame() *** _single_universe() * passes locally

test_dihedral.py reflects same changes as PR#218

7eff11f

cadeduckworth added a commit that referenced this pull request Nov 17, 2022

Merge remote-tracking branch 'origin/ensemble_run_update' into automa…

29477ab

…ted-dihedral-analysis updating with changes for backend and testing from PR #216 ensemble_run_update

cadeduckworth added a commit that referenced this pull request Nov 17, 2022

merged updates from PR #216 and PR #218

642e87e

orbeckst and others added 5 commits November 17, 2022 09:07

Merge branch 'develop' into ensemble_run_update

67261cb

Merge branch 'develop' into ensemble_run_update

2f63c3a

Merge branch 'develop' into ensemble_run_update

ce02b3b

updated source and html docs and CHANGELOG to reflect changes made fo…

cf4bd40

…r EnsembleAnalysis run methods to _single_frame and _single_universe, including new tests for NotImplementedError usage

merge with origin changes

f45fc6b

orbeckst requested changes Dec 8, 2022

View reviewed changes

cadeduckworth commented Dec 8, 2022

View reviewed changes

split EnsembleAnalysis run method test, one test for _single_frame, o…

1a90ed8

…ne test for _single_universe, tested with other error types to confirm coverage

cadeduckworth added 3 commits December 8, 2022 15:21

minor change to run method tests for proper use of example EnsembleAn…

7054bb5

…alysis

added and corrected use of sphinx markup for documentation, edited ve…

19f7c84

…rsion information in CHANGES

changes to docs for sphinx markup

40381aa

orbeckst requested changes Dec 9, 2022

View reviewed changes

whitespace and docstring formatting

8870e39

orbeckst approved these changes Dec 12, 2022

View reviewed changes

orbeckst merged commit e395ac7 into develop Dec 12, 2022

orbeckst deleted the ensemble_run_update branch December 12, 2022 20:33

		* source and html docs updated for EnsembleAnalysis
		to reflect changes and describe proper usage

		systems. This allows for users to easily run analyses on MDPOW simulations.

		NotImplementedError will detect whether _single_universe or _single_frame

	NotImplementedError will detect whether _single_universe or _single_frame
	:exc:`NotImplementedError` will detect whether :meth:`_single_universe` or :meth:`_single_frame`

		@@ -509,15 +517,8 @@ def run(self, start=None, stop=None, step=None):
		on each frame in the system.

		First iterates through keys of ensemble, then runs _setup_system

		msg = '''Dihedral calculations require AtomGroups with
		only 4 atoms, %s selected''' % len(group)

developing back end run methods #216

developing back end run methods #216

Conversation

cadeduckworth commented Sep 15, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Oct 20, 2022

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

cadeduckworth Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cadeduckworth Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

orbeckst commented Dec 8, 2022

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Dec 12, 2022

codecov bot commented Oct 28, 2022 •

edited

Loading

cadeduckworth Nov 17, 2022 •

edited

Loading

orbeckst left a comment •

edited

Loading

cadeduckworth Dec 8, 2022 •

edited

Loading