Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ensemble_wrapper decorator function #199

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/sphinx/source/analysis/ensemble.txt
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,9 @@ object when the :meth:`~mdpow.analysis.ensemble.EnsembleAtomGroup.ensemble` is r

.. autoclass:: mdpow.analysis.ensemble.EnsembleAtomGroup
:members:


ensemble_wrapper Decorator
__________________________

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more text here that shows a developer how to use it. Perhaps based on a real world example using some of the simpler MDAnalysis base classes?

There should be enough information for another developer (or future you) to learn how to use it. This means answers to the following questions:

  1. What problem is the code going to solve?
  2. What is the advantage of doing it this way?
  3. What do I need to provide?
  4. How do I do it?
  5. What are the limitations?

.. autofunction:: mdpow.analysis.ensemble_wrapper
47 changes: 46 additions & 1 deletion mdpow/analysis/ensemble.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import os
import errno
from typing import Optional, List
from typing import Optional, List, Union

import numpy as np

Expand Down Expand Up @@ -550,3 +550,48 @@ def check_groups_from_common_ensemble(groups: List[EnsembleAtomGroup]):
from the same Ensemble.'''
logger.error(msg)
raise ValueError(msg)


def ensemble_wrapper(cls):
"""A decorator for :class:`MDAnalysis.Universe <MDAnalysis.analysis.base.AnalysisBase>` subclasses modifying
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a long 1-line string – shorten

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and add this in the body

them to accept an :class:`~mdpow.analysis.ensemble.Ensemble` or
:class:`~mdpow.analysis.ensemble.EnsembleAtomGroup`.

.. rubric:: Example Analysis

@ensemble_wrapper
class Example(AnalysisBase):
pass
Comment on lines +563 to +564
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pass class is too trivial, it does not show how you deal with AtomGroups and whatever else one might want to know. It just shows how to use a decorator. Less trivial example here and perhaps a proper example in the text outside the function.


Ens = Ensemble(dirname='mol_dir)
ExRun = Example(Ens)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call the instance ex or something else in lower case for an instance.


"""
class EnsembleWrapper:
def __init__(self, ensemble: Union[Ensemble, EnsembleAtomGroup], *args, **kwargs):
self._ensemble = ensemble
self._args = args
self._kwargs = kwargs
self._Analysis = cls

def _prepare_ensemble(self):
# Defined separately so user can modify behavior
self._results_dict = {x: None for x in self._ensemble.keys()}

def _conclude_system(self):
# Defined separately so user can modify behavior
self._results_dict[self._key] = self._SystemRun.results

def _conclude_ensemble(self):
self.results = self._results_dict

def run(self, start=0, stop=0, step=1):
self._prepare_ensemble()
for self._key in self._ensemble.keys():
self._SystemRun = self._Analysis(self._ensemble[self._key], *self._args, **self._kwargs)
self._SystemRun.run(start=start, step=step, stop=stop)
self._conclude_system()
self._conclude_ensemble()
return self
Comment on lines +577 to +595
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like code duplication – my bigger conceptual problem is that a wrapped AnalysisBase is not an instance of EnsembleAnalysis.


return EnsembleWrapper
29 changes: 28 additions & 1 deletion mdpow/tests/test_ensemble.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@

import MDAnalysis as mda
from MDAnalysis.exceptions import NoDataError, SelectionError
from MDAnalysis.analysis.base import AnalysisBase

from gromacs.utilities import in_dir

from ..analysis.ensemble import Ensemble, EnsembleAnalysis, EnsembleAtomGroup
from ..analysis.ensemble import Ensemble, EnsembleAnalysis, EnsembleAtomGroup, ensemble_wrapper
from ..analysis.dihedral import DihedralAnalysis

from pkg_resources import resource_filename
Expand Down Expand Up @@ -161,3 +162,29 @@ def test_value_error(self):
dh4 = ens.select_atoms('name C4 or name C17 or name S2 or name N3')
with pytest.raises(ValueError):
dh_run = DihedralAnalysis([dh1, dh2, dh4, dh3]).run(start=0, stop=4, step=1)

def test_ensemble_wrapper1(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the "1" ?


class BaseTest(AnalysisBase):
def __init__(self, system: mda.Universe):
super(BaseTest, self).__init__(system.trajectory)
self.system = system
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call it universe instead of system? That would be clearer.


def _prepare(self):
self._res_arr = []

def _single_frame(self):
self._res_arr.append(len(self.system.select_atoms('not resname SOL')))
assert self._res_arr[-1] == 42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there an assert in AnalysisBase?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to say that you know that there are 42 atoms in the solute. However, this assert looks as if you wanted it to be part of the test so it's confusing. At a minimum, add a comment.

But perhaps you could just make it a more realistic analysis? People also look at tests to see how to use code so giving a good example here is useful. And you could use it for the docs as well.


def _conclude(self):
self.results = self._res_arr

@ensemble_wrapper
class EnsembleTest(BaseTest):
pass

Sim = Ensemble(dirname=self.tmpdir.name, solvents=['water'])
SolvCount = EnsembleTest(Sim).run(stop=10)
assert isinstance(SolvCount, EnsembleTest)
assert SolvCount.results[('water', 'VDW', '0000')] == ([42] * 10)