markovmodel / PyEMMA

🚂 Python API for Emma's Markov Model Algorithms 🚂
http://pyemma.org
GNU Lesser General Public License v3.0
311 stars 119 forks source link

Documented keyword indices2 for featurizer.add_distances turns out to be unexpected #399

Closed cwehmeyer closed 8 years ago

cwehmeyer commented 9 years ago

The pyemma documentation http://pythonhosted.org/pyEMMA/api/generated/pyemma.coordinates.featurizer.html#pyemma.coordinates.data.featurizer.MDFeaturizer.add_distances documents the keyword indices2. Using this keyword, however, yields the error message featurizer.add_distances(selection_C7, indices2=selection_CA) TypeError: add_distances() got an unexpected keyword argument 'indices2'.

gph82 commented 9 years ago

HI Chris, this is really weird, we tested this, I think...Looking into it right now

gph82 commented 9 years ago

Hi @marscher , this commit 4ab8f608ac3b8a2d9f91e26b2bc175699c7f5628 seems to be lost in my current dev_release_1.3 brach. @cwehmeyer , what branch are you on?

franknoe commented 9 years ago

Another missing thing? I am a bit worried that the rebase to devel didn't work properly. We cannot track down individual commits. So how can we make sure that all devel things made it into dev_release_1.3 properly?

Am 08/07/15 um 14:10 schrieb Guillermo Pérez-Hernández:

Hi @marscher https://github.com/marscher , this commit 4ab8f60 https://github.com/markovmodel/PyEMMA/commit/4ab8f608ac3b8a2d9f91e26b2bc175699c7f5628 seems to be lost in my current dev_release_1.3 brach. @cwehmeyer https://github.com/cwehmeyer , what branch are you on?

— Reply to this email directly or view it on GitHub https://github.com/markovmodel/PyEMMA/issues/399#issuecomment-119553331.


Prof. Dr. Frank Noe Head of Computational Molecular Biology group Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354 Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

gph82 commented 9 years ago

Well, give me more time look more carefully at this, but yes, the rebase is causing some headaches

cwehmeyer commented 9 years ago

@gph82 I'm using pyemma-1.2.1 and installed from the omnia channel.

franknoe commented 9 years ago

Ah that explains it. The docs are generated for the devel branch and this feature is not in a release yet.

Am 08/07/15 um 14:19 schrieb Christoph Wehmeyer:

@gph82 https://github.com/gph82 I'm using pyemma-1.2.1 and installed from the omnia channel.

— Reply to this email directly or view it on GitHub https://github.com/markovmodel/PyEMMA/issues/399#issuecomment-119555982.


Prof. Dr. Frank Noe Head of Computational Molecular Biology group Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354 Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

gph82 commented 9 years ago

Okay,

dev_release_1.3:

https://github.com/markovmodel/PyEMMA/blob/dev_release_1.3/pyemma/coordinates/data/featurizer.py/#L752-784

and devel: https://github.com/markovmodel/PyEMMA/blob/devel/pyemma/coordinates/data/featurizer.py/#L752-784

have the correct code.

So I don't know about omia. It is also pretty strange that the documentation is there but the code not

 def add_distances(self, indices, periodic=True, indices2=None):
        r"""
        Adds the distances between atoms to the feature list.
        Parameters
        ----------
        indices : can be of two types:
                ndarray((n, 2), dtype=int):
                    n x 2 array with the pairs of atoms between which the distances shall be computed
                iterable of integers (either list or ndarray(n, dtype=int)):
                    indices (**not pairs of indices**) of the atoms between which the distances shall be computed.
                    Note that this will produce a pairlist different from the pairlist produced by :py:func:`pairs` in that this **does not** exclude
                    1-2 neighbors.
        indices2: iterable of integers (either list or ndarray(n, dtype=int)), optional:
                    Only has effect if :py:obj:`indices` is an iterable of integers. Instead of the above behaviour,
                    only the distances between the atoms in :py:obj:`indices` and :py:obj:`indices2` will be computed.
        .. note::
            When using the *iterable of integers* input, :py:obj:`indices` and :py:obj:`indices2`
            will be sorted numerically and made unique before converting them to a pairlist.
            Please look carefully at the output of :py:func:`describe()` to see what features exactly have been added.
        """

        atom_pairs = _parse_pairwise_input(
            indices, indices2, self._logger, fname='add_distances()')

        atom_pairs = self._check_indices(atom_pairs)
        f = DistanceFeature(self.topology, atom_pairs, periodic=periodic)
        self.__add_feature(f)
gph82 commented 9 years ago

How do we fix this mismatch?

franknoe commented 9 years ago

we don't need to fix it, we just need to make clear in the docs that the devel branch is documented. In other python projects the docs have labels like "latest" (for devel) "1.2" (for release 1.2) etc. Maybe we should do that too

Am 08/07/15 um 14:21 schrieb Guillermo Pérez-Hernández:

How do we fix this mismatch?

— Reply to this email directly or view it on GitHub https://github.com/markovmodel/PyEMMA/issues/399#issuecomment-119556285.


Prof. Dr. Frank Noe Head of Computational Molecular Biology group Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354 Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

gph82 commented 9 years ago

Just to be clear, we are talking about the case where somebody is looking at the online documentation instead of the one that's shipped with their package?

franknoe commented 9 years ago

I was talking about the online docs

Am 08/07/15 um 14:26 schrieb Guillermo Pérez-Hernández:

Just to be clear, we are talking about the case where somebody is looking at the online documentation instead of the one that's shipped with their package?

— Reply to this email directly or view it on GitHub https://github.com/markovmodel/PyEMMA/issues/399#issuecomment-119557524.


Prof. Dr. Frank Noe Head of Computational Molecular Biology group Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354 Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

cwehmeyer commented 9 years ago

I would like having separate online docs for releases and the devel branch.

gph82 commented 9 years ago

Agree, @cwehmeyer , but even the function call would be very informative, right? Either with the notebook's autocomplete our with the "?" from ipython console, you could see that the call in your version does not have the argument indices2...

marscher commented 9 years ago

After a short discussion with @gph82 and @cwehmeyer we agreed to use ReadTheDocs, which requires a little work (we have to fake all external dependencies and can not compile any C-extensions). This way we would have all the prior versions available. The issue has arisen because we're currently only building the docs from the state of the devel branch, which of course diverged from latest released version.

franknoe commented 9 years ago

ok

Am 15/07/15 um 15:06 schrieb Martin K. Scherer:

After a short discussion with @gph82 https://github.com/gph82 and @cwehmeyer https://github.com/cwehmeyer we agreed to use ReadTheDocs, which requires a little work (we have to fake all external dependencies and can not compile any C-extensions). This way we would have all the prior versions available. The issue has arisen because we're currently only building the docs from the state of the devel branch, which of course diverged from latest released version.

— Reply to this email directly or view it on GitHub https://github.com/markovmodel/PyEMMA/issues/399#issuecomment-121609162.


Prof. Dr. Frank Noe Head of Computational Molecular Biology group Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354 Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

marscher commented 9 years ago

I also requested a feature to upload pre-built docs in rtfd/readthedocs.org/issues/1083 but so far they have not made a decision yet.

franknoe commented 9 years ago

Why do we need this? Isn't their service to build the docs?

Am 15/07/15 um 15:28 schrieb Martin K. Scherer:

I also requested a feature to upload pre-built docs in rtfd/readthedocs.org#1083 https://github.com/rtfd/readthedocs.org/issues/1083 but so far they have not made a decision yet.

— Reply to this email directly or view it on GitHub https://github.com/markovmodel/PyEMMA/issues/399#issuecomment-121616973.


Prof. Dr. Frank Noe Head of Computational Molecular Biology group Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354 Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

marscher commented 9 years ago

Am 15.07.2015 um 15:44 schrieb Frank Noe:

Why do we need this? Isn't their service to build the docs? yes, but they do not allow installing dependencies with extensions, which is a blocker in our case.

At the end of the week we'll have access to our new Jenkins service, where we can implement the build and publishing on FU servers and have full control over whole process.

marscher commented 8 years ago

done.