add deprecation warnings for API changes #1403

orbeckst · 2017-06-15T23:36:20Z

Changes made in this Pull Request:

closes add deprecation warnings for API changes #1383
included deprecations:
- Remove TimeSeries #1373 Timeseries (targeted for 0.17)
  Note that the deprecation for core.Timeseries will always show up;
  this is deliberate so that users WILL see it as it will be gone in
  the next release!
- Remove instant selectors #1377 Quick selectors (target 1.0)
- remove flag registry #782 flags (target 1.0)
updated CHANGELOG

PR Checklist

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

kain88-de · 2017-06-16T08:01:33Z

package/MDAnalysis/coordinates/memory.py

@@ -106,6 +106,8 @@
    universe2 = mda.Universe(PSF, coordinates, format=MemoryReader, order='afc')


+.. _create-in-memory-trajectory-with-AnalysisFromFunction:


why was this added?

So that I can reference the anchor in the docs in package/MDAnalysis/core/Timeseries.py – it can be removed again in 0.17.0 when Timeseries.py is gone. This is the standard way to do cross references in sphinx.

I do not like the idea of removing the docs (see below) but rather point to alternatives. This will be the last release with the feature. If you ever need to tell someone which release to use then you point them to 0.16.2. And then the docs for the feature should still exist.

kain88-de · 2017-06-16T08:03:09Z

package/MDAnalysis/core/Timeseries.py

+    warnings.warn(('The Timeseries module and TimeseriesCollection will be '
+                   'removed in the 0.17 release. See issue #1372 '
+                   'https://github.com/MDAnalysis/mdanalysis/issues/1373'),
+                  DeprecationWarning)


This leaves the warning levels of other modules like ipython untouched?

I presume so... it's what we have been doing elsewhere.

In [15]: warnings.catch_warnings? Init signature: warnings.catch_warnings(self, record=False, module=None) Docstring: A context manager that copies and restores the warnings filter upon exiting the context.

Good just checking

kain88-de · 2017-06-16T08:04:32Z

package/MDAnalysis/core/__init__.py

@@ -30,7 +30,8 @@
 :class:`~MDAnalysis.core.groups.AtomGroup` and return another
 :class:`~MDAnalysis.core.groups.AtomGroup`.

-:mod:`~MDAnalysis.Timeseries` are a convenient way to analyse trajectories.
+:mod:`~MDAnalysis.core.Timeseries` are a convenient way to analyse
+trajectories.


Can you remove this due to deprecations already here? We should already point people to use the analysis frame work

Ok, will do.

kain88-de · 2017-06-16T08:05:37Z

package/MDAnalysis/core/groups.py

    """
    def __getitem__(self, item):
+        # DEPRECATED in 0.16.2
+        # REMOVE in 1.0
+        #


shouldn't there be warnings issues here?

This will annoy the hell of people, won't it? I can't use the decorator because __getitem__ is used for other stuff.

Ok, adding, the warnings only show up once.

Well we only have a warning when item is a string right?

richardjgowers · 2017-06-16T08:59:23Z

package/MDAnalysis/core/groups.py

+    *Instant selectors* will be removed in the 1.0 release.  See issue `#1377
+    <https://github.com/MDAnalysis/mdanalysis/issues/1377>`_ for more details.
+
+    Atoms can also be accessed in a Pythonic fashion by using the atom name as


If we're deprecating something, can we remove the documentation for it? Someone might skip the dep warning, then read about this great feature they want to use forevermore....

I don't want to remove docs of a feature in the last release where the feature exists. If anyone really wants the feature then they should still have the docs. Deprecation warnings should be enough. And it's already in its own section.

I confused the instant selectors with the Timeseries regarding "last release". But I still think that it should be documented for as long as it is available.

(Otherwise the deprecated:: 0.16.2 tag makes little sense: it says when it was deprecated, not when it was removed. The removal also kills the docs and possible leaves a versionchanged:: 1.0.0 behind.)

orbeckst · 2017-06-16T19:06:52Z

@richardjgowers how do ResidueGroup and SegmentGroup do their instant selector magic?

import MDAnalysis as mda; from MDAnalysisTests.datafiles import PSF
u = mda.Universe(PSF)

print(u.residues.LYS)
print(u.segments.s4AKE)

I don't see a __getattr__ in them or in GroupBase so I don't know how to attach deprecation warnings.

kain88-de · 2017-06-16T20:40:53Z

OK my pushes are now showing up in the PR's that is weird

orbeckst · 2017-06-16T20:53:53Z

Thanks, I will merge with what I am working on.

richardjgowers · 2017-06-16T21:17:52Z

@orbeckst

You should be able to use the @deprecate decorator on those and it'll only appear if you use them

orbeckst · 2017-06-16T21:28:31Z

@richardjgowers how does this end up in __getattr__?

orbeckst · 2017-06-16T21:29:25Z

... or have a look at the PR and comment, that is probably going to be more helpful. Or push to the PR.

I have to do other things for the rest of the day, sorry.

orbeckst · 2017-06-16T21:33:12Z

Note: At least two of the new tests are currently failing because I didn't know how add depreactions:

MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_AtomGroup_nowarn_getitem_index ... ok
MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_AtomGroup_warn_getattr ... ok
MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_AtomGroup_warn_getitem ... ok
MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_ResidueGroup_warn_getattr_resname ... FAIL
MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_SegmentGroup_nowarn_getitem ... ok
MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_SegmentGroup_warn_getattr ... FAIL
MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_Segment_warn_getattr_rRESNUM ... ok
MDAnalysisTests.core.test_groups.TestInstantSelectorDeprecationWarnings.test_Segment_warn_getattr_resname ... ok

orbeckst · 2017-06-17T00:51:23Z

I am trying #1403 (comment).

@richardjgowers

Thanks to @richardjgowers for insights #1403 (comment)

orbeckst · 2017-06-17T01:03:50Z

With decorators on the methods in #1403 (comment) and explicit warnings in one other place (for the Segment.r1 selectors) it now seems to be working as expected, at least locally.

I will rebase and clean up.

orbeckst · 2017-06-17T01:09:11Z

Interactive work will be annoying for a while because TAB completion on ``u.residues.` will trigger the deprecation warning. Depending on settings for the warning filter, this will only show up once, though, and then deprecation warnings for real use will be swallowed. Let's see how it goes... if it is too annoying we can fix it (... how? ...) as we go along.

@richardjgowers

- instant selectors AtomGroup['<name>'], AtomGroup.<name>, ResidueGroup.<name>, Segment.<segid>, Segment.r<resid>, SegmentGroup.<segid> emit warnings - added tests - updated docs with detailed deprecation and alternatives Thanks to @richardjgowers for insights #1403 (comment)

orbeckst · 2017-06-17T01:12:56Z

Rebased and force-pushed.

If travis is green (any my local docs look ok) then this is good to go from my side.

orbeckst · 2017-06-17T01:15:33Z

sphinx fails, not sure why:

/Volumes/Data/oliver/Biop/Projects/Methods/MDAnalysis/mdanalysis/package/MDAnalysis/core/Timeseries.py:82: DeprecationWarning: The Timeseries module and TimeseriesCollection will be removed in the 0.17 release. See issue #1372 https://github.com/MDAnalysis/mdanalysis/issues/1373
  DeprecationWarning)
reading sources... [100%] index

Warning, treated as error:
/Volumes/Data/oliver/Biop/Projects/Methods/MDAnalysis/mdanalysis/package/MDAnalysis/coordinates/DCD.py:docstring of MDAnalysis.coordinates.DCD.DCDReader.correl:11: (SEVERE/4) Unexpected section title.

Parameters
----------

The only change is @deprecate decorator on DCD.correl.

kain88-de · 2017-06-17T06:36:25Z

I fixed all sphinx warnings.

kain88-de · 2017-06-17T06:37:01Z

If you use the numpy deprecate decorator sphinx might fail because the decorator changes the docstring. This seems to be problematic for class member functions.

kain88-de · 2017-06-17T09:16:34Z

Can someone else please have a look at the failing tests. I don't find where we trigger the warnings that fail for the old numpy version build.

kain88-de · 2017-06-17T10:02:44Z

package/MDAnalysis/core/topologyattrs.py

@@ -409,6 +411,9 @@ def getattr__(atomgroup, name):
            raise AttributeError("'{0}' object has no attribute '{1}'".format(
                    atomgroup.__class__.__name__, name))

+    @deprecate(message="Instant selector AtomGroup['<name>'] or AtomGroup.<name> "
+               "is deprecated and will be removed in 1.0. "
+               "Use AtomGroup.select_atoms('name <name>') instead.")
    def _get_named_atom(group, name):


We now issue a warning for atoms.ChainIDs or atoms.segids. I don't think that was the indented effect. @richardjgowers could you have a look please.

@kain88-de I added a test if AtomGroup.segids raises a warning and locally the test passes.

In python and ipython, no warning is raised for atoms.segids:

Python 2.7.13 (default, Apr 25 2017, 03:00:22) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import MDAnalysis as mda; from MDAnalysisTests.datafiles import PSF; import MDAnalysisTests >>> w = MDAnalysisTests.make_Universe(("resids", "resnames", "segids", "names")) >>> w <Universe with 125 atoms> >>> w.atoms.segids array(['SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegA', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegB', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegC', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegD', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE', 'SegE'], dtype=object) >>> >>> w.atoms['AAA'] /Volumes/Data/oliver/Biop/Projects/Methods/MDAnalysis/mdanalysis/package/MDAnalysis/core/groups.py:1520: DeprecationWarning: Using the [] operator with strings is deprecated and will be removed in 1.0. Please use `select_atoms('name AAA')` instead. category=DeprecationWarning) /Volumes/Data/oliver/Biop/Projects/Methods/MDAnalysis/mdanalysis/package/MDAnalysis/core/groups.py:1522: DeprecationWarning: `_get_named_atom` is deprecated! Instant selector AtomGroup['<name>'] or AtomGroup.<name> is deprecated and will be removed in 1.0. Use AtomGroup.select_atoms('name <name>') instead. return self._get_named_atom(item) <Atom 1: AAA of resname RsA, resid 1 and segid SegA>

Could you please paste what code you've been running?

I adopted _get_named_atom to also print the requested name. That way I found out we apparently use it for more then just atom names

from MDAnalysisTests import make_Universe import warnings warnings.simplefilter('always') req_attrs = {'resnames': 'UNK', 'resids': 1, 'names': 'X', 'tempfactors': 0.0, } attrs = list(req_attrs.keys()) u = make_Universe(attrs, trajectory=True) u.atoms.write('test.crd')

output

/home/max/foss/molecular-dynamics/mdanalysis/mdanalysis/package/MDAnalysis/core/groups.py:1539: DeprecationWarning: `_get_named_atom` is deprecated! Instant selector AtomGroup['<name>'] or AtomGroup.<name> is deprecated and will be removed in 1.0. Use AtomGroup.select_atoms('name <name>') instead. return self._get_named_atom(attr) chainIDs /home/max/foss/molecular-dynamics/mdanalysis/mdanalysis/package/MDAnalysis/core/groups.py:1539: DeprecationWarning: `_get_named_atom` is deprecated! Instant selector AtomGroup['<name>'] or AtomGroup.<name> is deprecated and will be removed in 1.0. Use AtomGroup.select_atoms('name <name>') instead. return self._get_named_atom(attr) segids /home/max/foss/molecular-dynamics/mdanalysis/mdanalysis/package/MDAnalysis/coordinates/CRD.py:229: UserWarning: Supplied AtomGroup was missing the following attributes: tempfactors. These will be written with default values. "".format(miss=', '.join(missing_topology)))

This is running with numpy 1.12.1

orbeckst · 2017-06-17T23:04:54Z

package/MDAnalysis/core/groups.py

@@ -1513,6 +1513,9 @@ def __getitem__(self, item):
        #
        # u.atoms['HT1'] access, otherwise default
        if isinstance(item, string_types):
+            warnings.warn("Using the [] operator with strings is deprecated."


@kain88-de thanks for adding it back – I must have removed it accidentally when I rebased.

orbeckst · 2017-06-17T23:07:09Z

@kain88-de thanks for working on the PR and figuring out the deprecation decorator/sphinx issue as well as putting back 13bc527.

See comment #1403 (comment)

orbeckst · 2017-06-18T01:10:13Z

#1403 (comment)

Can someone else please have a look at the failing tests. I don't find where we trigger the warnings that fail for the old numpy version build.

It is weird that the OLD_NUMPY tests fail but the new ones do not – if anything, the new ones should fail, too! It seems that _get_named_atom() is not only used for instant selectors but also for other things, e.g., in the CDRWriter or PQRWriter. https://travis-ci.org/MDAnalysis/mdanalysis/jobs/244102138#L1310

What should we do? Create a copy of _get_named_atom() and deprecate one? But I would need @richardjgowers help in figuring out where this goes. I don't fully understand the new topology magic yet...

- closes #1383 - included deprecations: - #1373 Timeseries (targeted for 0.17) Note that the deprecation for core.Timeseries will always show up; this is deliberate so that users WILL see it as it will be gone in the next release! - #1377 Quick selectors (target 1.0) - #782 flags (target 1.0) - updated CHANGELOG

- improved installation instructions (minimal/full install for pip, always full install for conda; also added entry on installation of tests) - analysis landing page: better explained optional packages, removed scipy as example and replaced with sklearn and HOLE.

- removed imports in core and MDAnalysis - replaced MDAnalysis.collection with a mock object that issues warnings and raises NotImplementedError - added a test for MDAnalysis.collection

@richardjgowers

- instant selectors AtomGroup['<name>'], AtomGroup.<name>, ResidueGroup.<name>, Segment.<segid>, Segment.r<resid>, SegmentGroup.<segid> emit warnings - added tests - updated docs with detailed deprecation and alternatives Thanks to @richardjgowers for insights #1403 (comment)

See comment #1403 (comment)

It turns out that with this warning in place we raise two deprecation warnings because we also have a warning on _get_named_atom(item). Example output: >>> w.atoms['AAA'] .../mdanalysis/package/MDAnalysis/core/groups.py:1520: DeprecationWarning: Using the [] operator with strings is deprecated and will be removed in 1.0. Please use `select_atoms('name AAA')` instead. category=DeprecationWarning) .../mdanalysis/package/MDAnalysis/core/groups.py:1522: DeprecationWarning: `_get_named_atom` is deprecated! Instant selector AtomGroup['<name>'] or AtomGroup.<name> is deprecated and will be removed in 1.0. Use AtomGroup.select_atoms('name <name>') instead. return self._get_named_atom(item) <Atom 1: AAA of resname RsA, resid 1 and segid SegA>

This allows code to use the `getattr` function and only throw an error if the requested attribute is an atom name.

kain88-de · 2017-06-20T14:21:17Z

@richardjgowers if this solves the problem we might be able to just do a normal style release.

kain88-de · 2017-06-20T15:00:28Z

It seems to have worked. The old numpy test isn't failing anymore.

kain88-de · 2017-06-20T15:35:15Z

@richardjgowers do you want to merge this?

@jbarnoud did you see anything else that needs to be fixed?

richardjgowers · 2017-06-20T15:48:14Z

I should probably clone it and play with it locally to check it. Give me a few hours to testdrive it

orbeckst · 2017-06-20T23:27:18Z

The full test times out, everything else seems to be ok. @richardjgowers you have the honor of merging...

@richardjgowers

- instant selectors AtomGroup['<name>'], AtomGroup.<name>, ResidueGroup.<name>, Segment.<segid>, Segment.r<resid>, SegmentGroup.<segid> emit warnings - added tests - updated docs with detailed deprecation and alternatives Thanks to @richardjgowers for insights #1403 (comment)

See comment #1403 (comment)

orbeckst added this to the 0.16.x milestone Jun 15, 2017

orbeckst requested review from kain88-de and richardjgowers June 15, 2017 23:36

orbeckst mentioned this pull request Jun 16, 2017

Adds Reader.timeseries #1400

Closed

4 tasks

kain88-de mentioned this pull request Jun 16, 2017

release 0.16.2 #1401

Closed

11 tasks

kain88-de reviewed Jun 16, 2017

View reviewed changes

richardjgowers reviewed Jun 16, 2017

View reviewed changes

orbeckst added a commit that referenced this pull request Jun 17, 2017

warnings for ALL instant selectors

eca6a61

Thanks to @richardjgowers for insights #1403 (comment)

orbeckst force-pushed the issue-1383-deprecation-warnings branch from eca6a61 to 3acba3c Compare June 17, 2017 01:12

kain88-de reviewed Jun 17, 2017

View reviewed changes

orbeckst commented Jun 17, 2017

View reviewed changes

orbeckst added a commit that referenced this pull request Jun 17, 2017

test for no warnings raised by AtomGroup.segid

1a4ff65

See comment #1403 (comment)

orbeckst and others added 15 commits June 20, 2017 16:20

deprecated core.Timeseries module

cecba37

Deprecated DCDReader.correl

34fc6fc

removed top-level imports of Timeseries

8668259

- removed imports in core and MDAnalysis - replaced MDAnalysis.collection with a mock object that issues warnings and raises NotImplementedError - added a test for MDAnalysis.collection

new MDAnalysisTests.core.util.assert_nowarns() function

bba1d85

fix sphinx warnings

7c4a98f

add getitem warning message

a9b1fc3

use explicit deprecation warning

2add693

test for no warnings raised by AtomGroup.segid

69a4750

See comment #1403 (comment)

minor cleanups in tests

bf6bc70

AtomGroup.__getitem__ depreaction warning: added removal release

6d8a520

only issue _get_named_atom warning on successful call

a9f9655

This allows code to use the `getattr` function and only throw an error if the requested attribute is an atom name.

kain88-de force-pushed the issue-1383-deprecation-warnings branch from 1abebb2 to a9f9655 Compare June 20, 2017 14:20

jbarnoud approved these changes Jun 20, 2017

View reviewed changes

richardjgowers approved these changes Jun 20, 2017

View reviewed changes

orbeckst assigned richardjgowers Jun 20, 2017

richardjgowers merged commit e24da32 into develop Jun 21, 2017

richardjgowers deleted the issue-1383-deprecation-warnings branch June 21, 2017 09:42

richardjgowers pushed a commit that referenced this pull request Jun 21, 2017

test for no warnings raised by AtomGroup.segid

e15f1be

See comment #1403 (comment)

This was referenced Jul 12, 2017

universe.segment and universe.residues triger a deprecation warning #1476

Closed

universe.<segment id> does not issue a deprecation warning #1478

Closed

		@@ -106,6 +106,8 @@
		universe2 = mda.Universe(PSF, coordinates, format=MemoryReader, order='afc')


		.. _create-in-memory-trajectory-with-AnalysisFromFunction:

add deprecation warnings for API changes #1403

add deprecation warnings for API changes #1403

Conversation

orbeckst commented Jun 15, 2017 • edited Loading

PR Checklist

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

orbeckst commented Jun 16, 2017

kain88-de commented Jun 16, 2017

orbeckst commented Jun 16, 2017

richardjgowers commented Jun 16, 2017

orbeckst commented Jun 16, 2017

orbeckst commented Jun 16, 2017

orbeckst commented Jun 16, 2017

orbeckst commented Jun 17, 2017

orbeckst commented Jun 17, 2017

orbeckst commented Jun 17, 2017

orbeckst commented Jun 17, 2017

orbeckst commented Jun 17, 2017

kain88-de commented Jun 17, 2017

kain88-de commented Jun 17, 2017

kain88-de commented Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

orbeckst Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Jun 17, 2017

orbeckst commented Jun 18, 2017

kain88-de commented Jun 20, 2017

kain88-de commented Jun 20, 2017

kain88-de commented Jun 20, 2017

richardjgowers commented Jun 20, 2017

orbeckst commented Jun 20, 2017

orbeckst commented Jun 15, 2017 •

edited

Loading

kain88-de commented Jun 17, 2017 •

edited

Loading

orbeckst Jun 17, 2017 •

edited

Loading