ENH: Add warning when setting into nonexistent attribute #16951

deniederhut · 2017-07-15T16:52:52Z

closes GOTCHA: You can't use dot notation to add a column to a DataFrame #7175
closes Suggestion: Warning for DataFrame colname-methodname clash #5904
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2017-07-15T16:52:55Z

Hello @deniederhut! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 04, 2017 at 12:16 Hours UTC

jreback · 2017-07-15T16:59:27Z

doc/source/indexing.rst


 You can use attribute access to modify an existing element of a Series or column of a DataFrame, but be careful;
-if you try to use attribute access to create a new column, it fails silently, creating a new attribute rather than a
+if you try to use attribute access to create a new column, it issues a `UserWarning` and creates a new attribute rather than a


use double-backticks around UserWarning

jreback · 2017-07-15T17:00:02Z

pandas/core/generic.py

+                if (self.ndim > 1) and (is_list_like(value)):
+                    warnings.warn("Pandas doesn't allow Series to be assigned "
+                                  "into nonexistent columns - see "
+                                  "https://pandas.pydata.org/pandas-docs/"


you need a stacklevel=2 (or maybe higher here)

This has been fixed -- not sure why GH isn't picking it up

use if isinstance(self, ABCDataFrame) and is_list_like(value):

this change here

jreback · 2017-07-15T17:00:52Z

pandas/tests/dtypes/test_generic.py

+        with catch_warnings(record=True) as w:
+            self.series.not_an_index = [1, 2]
+            assert len(w) == 0 # fail if false warning on Series
+        with pytest.warns(UserWarning):


use tm.assert_raises_warning

jreback · 2017-07-15T17:01:20Z

pandas/tests/dtypes/test_generic.py

        assert isinstance(self.sparse_array, gt.ABCSparseArray)
        assert isinstance(self.categorical, gt.ABCCategorical)
        assert isinstance(pd.Period('2012', freq='A-DEC'), gt.ABCPeriod)
+        with catch_warnings(record=True) as w:


what are you testing here?

jreback · 2017-07-15T17:01:31Z

pandas/tests/dtypes/test_generic.py

        assert isinstance(pd.Period('2012', freq='A-DEC'), gt.ABCPeriod)
+        with catch_warnings(record=True) as w:
+            self.series.not_an_index = [1, 2]
+            assert len(w) == 0 # fail if false warning on Series


move this to a new test add the issue number as a comment

jreback · 2017-07-15T17:03:01Z

Replicate this test from the original issue (and assert each of these things).

d = {'one' : pd.Series([1., 2., 3.], index=['a', 'b', 'c']),
     'two' : pd.Series([1., 2., 3., 4.], index=['a', 'b', 'c', 'd'])}
df = pd.DataFrame(d)

# this adds a column to df
df['three'] = df.two + 1

# this successfully modifies an existing column
df.one += 1

# this seems to work but does not add a column to df
df.four = df.two + 2

print df.three
print df.four
print df

Here is the warning.IOW assignment with a pandas object to a non-existing attribute (actually would be ok with this raising I think), though this might break people downstream.

# this seems to work but does not add a column to df
df.four = df.two + 2

jreback · 2017-07-15T17:04:33Z

also would love to warn on this: #5904 as well.

deniederhut · 2017-07-15T17:04:35Z

Wilco. Still in test_generic.py?

jreback · 2017-07-15T17:25:43Z

Wilco. Still in test_generic.py?

yeah that seems reasonable

codecov · 2017-07-15T17:28:41Z

Codecov Report

Merging #16951 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16951      +/-   ##
==========================================
- Coverage   90.98%   90.96%   -0.02%     
==========================================
  Files         161      161              
  Lines       49288    49290       +2     
==========================================
- Hits        44846    44839       -7     
- Misses       4442     4451       +9

Flag	Coverage Δ
#multiple	`88.74% <100%> (ø)`	⬆️
#single	`40.2% <100%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/generic.py	`92.3% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.76% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c498f8...cfe1bad. Read the comment docs.

codecov · 2017-07-15T17:28:52Z

Codecov Report

Merging #16951 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16951      +/-   ##
==========================================
- Coverage   90.99%   90.98%   -0.02%     
==========================================
  Files         162      162              
  Lines       49508    49512       +4     
==========================================
- Hits        45052    45047       -5     
- Misses       4456     4465       +9

Flag	Coverage Δ
#multiple	`88.76% <100%> (ø)`	⬆️
#single	`40.27% <40%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/generic.py	`92.03% <100%> (+0.01%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.66% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 929c66f...b86546e. Read the comment docs.

jreback · 2017-07-15T17:34:22Z

doc/source/indexing.rst


 You can use attribute access to modify an existing element of a Series or column of a DataFrame, but be careful;
-if you try to use attribute access to create a new column, it fails silently, creating a new attribute rather than a
+if you try to use attribute access to create a new column, it issues a ```UserWarning`` and creates a new attribute rather than a


maybe add a: starting in 0.21.0

jreback · 2017-07-15T18:51:39Z

pandas/core/generic.py

    def _set_item(self, key, value):
+        if callable(getattr(self, key, None)):
+            warnings.warn("Pandas doesn't allow attribute-like access to "
+                          "columns whose names collide with methods",


i would say that this is a collision

we actually DO allow it (maybe should raise but let's start with a warning)

The warning says that attribute-like access is not allowed. E.g.

df = pd.DataFrame({'a': [1, 2]}) df['sum'] = [3, 4] print(df.sum)

<bound method DataFrame.sum of a sum 0 1 3 1 2 4>

i just want to see a more verbose message that this is not recommended
we could also raise but that's a bit harsh

Okay... how about something like:

Column name '{key}' collides with a built-in method, which will cause unexpected attribute behavior

jreback · 2017-07-15T18:51:58Z

pandas/tests/dtypes/test_generic.py

    df = pd.DataFrame({'names': ['a', 'b', 'c']}, index=multi_index)
    sparse_series = pd.Series([1, 2, 3]).to_sparse()
    sparse_array = pd.SparseArray(np.random.randn(10))
+    series = pd.Series([1, 2, 3])


u can remove this

jreback · 2017-07-15T18:52:27Z

pandas/tests/dtypes/test_generic.py

        assert isinstance(pd.Period('2012', freq='A-DEC'), gt.ABCPeriod)
+
+
+class TestABCWarnings(object):


u don't need a class here
u can just do this as a function (more pytest idiomatic)

I didn't see anything in the contributing guide about test style, so I was following the style of the code nearby.

http://pandas.pydata.org/pandas-docs/stable/contributing.html#test-driven-development-code-writing

pytest section -

jreback · 2017-07-15T18:52:56Z

pandas/tests/dtypes/test_generic.py

+            self.df['three'] = self.df.two + 1
+            assert len(w) == 0
+            assert self.df.three.sum() > self.df.two.sum()
+        with catch_warnings(record=True) as w:


put blank lines in between these

jreback · 2017-07-15T18:53:31Z

pandas/tests/dtypes/test_generic.py

+            self.df.four = self.df.two + 2
+        with tm.assert_produces_warning(UserWarning):
+            # warn when column has same name as method
+            self.df['sum'] = self.df.two


test with self.df.sum as well

jreback · 2017-07-16T01:15:52Z

pandas/core/generic.py

        return result

    def _set_item(self, key, value):
+        if callable(getattr(self, str(key), None)):


check if its a string first, e.g.

if isinstance(key, compat.string_type) and callable(getattr(self, key, None))): ......

jreback · 2017-07-16T01:18:39Z

pandas/tests/dtypes/test_generic.py

+    df = pd.DataFrame(d)
+
+    with catch_warnings(record=True) as w:
+        # successfully add new column


add a statement that the first 3 should NOT be showing warnigns (I know you are checking which is fine)

jreback · 2017-07-16T01:19:25Z

@deniederhut looks pretty good. rebase on master, some changes and you prob need to force push.

jreback · 2017-07-16T01:20:16Z

also needs a whatsnew note; put this in the api-breaking changes section. I think we may need a full on example to highlite this.

deniederhut · 2017-07-19T00:18:20Z

Wilco.

And I might be missing something, but we are just issuing warnings, right? This shouldn't break any clients unless they are configured to raise warnings as exceptions?

jreback · 2017-07-19T00:36:19Z

yeah isn't it just a warning then no big deal

however I think we may want to consider making this an error in the future (so can make an issue for that)

deniederhut · 2017-07-19T00:49:58Z

Would you like me to add to the whatsnew that these warnings are subject to escalation in future releases?

jreback · 2017-07-19T00:52:40Z

no but pls open an issue to re-evaluate in the future

jreback

docs look good! some small changes.

jreback · 2017-07-19T01:25:41Z

doc/source/whatsnew/v0.21.0.txt

+  In[1]: df = pd.DataFrame({'one': [1., 2., 3.]})
+  In[2]: df.two = [4, 5, 6]
+
+which does not raise any obvious exceptions, but also does not create a new column:


which -> This

jreback · 2017-07-19T01:26:12Z

doc/source/whatsnew/v0.21.0.txt

+  1  2.0
+  2  3.0
+
+and creating a column whose name collides with a method or attribute already in the instance


Creating a column (use sentences if possible here).

jreback · 2017-07-19T01:28:08Z

doc/source/whatsnew/v0.21.0.txt

+.. code-block:: ipython
+
+  In[2]: df.two = [4, 5, 6]
+  UserWarning: Pandas doesn't allow Series to be assigned into nonexistent columns - see https://pandas.pydata.org/pandas-docs/stable/indexing.html#attribute-access


Can you put this whole section in indexing.rst (below the attribute access section). Put a reference to it from the whatsnew (you can leave some of it here as well; usually we make the whatsnew a smaller version of what we put in the docs proper).

jreback · 2017-07-19T01:28:30Z

pandas/core/generic.py

+                if (self.ndim > 1) and (is_list_like(value)):
+                    warnings.warn("Pandas doesn't allow Series to be assigned "
+                                  "into nonexistent columns - see "
+                                  "https://pandas.pydata.org/pandas-docs/"


this change here

deniederhut · 2017-07-19T12:11:07Z

Okay, I think I've gotten the docs correct

jbrockmendel · 2017-07-29T19:13:03Z

pandas/core/generic.py

        result._set_is_copy(self, copy=is_copy)
        return result

    def _set_item(self, key, value):


Is this a single-point-of-contact that all (most? many?) setter methods go through? i.e. will the various loc.__setitem__ paths eventually wind through here?

Yes, I believe that is correct.

That is my understanding, yes. Here is an example of setting while using .loc:

import pandas as pd df = pd.DataFrame({'one': [0, 1, 2]}) df.loc[:, 'sum'] = df.one.sum()

UserWarning: Column name 'sum' collides with a built-in method, which will cause unexpected attribute behavior self._set_item(key, value)

this point is only for [] setting, e.g. setting a column on a DF or an element on a Series. .loc/.iloc are handled in core/indexing.py

jreback

some doc comments. lgtm. ping on green for a final look.

jreback · 2017-07-31T23:20:16Z

doc/source/whatsnew/v0.21.0.txt

+.. _whatsnew_0210.enhancements.column-creation:
+
+Improved warnings when attempting to create columns
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


the underline length must match the title.

jreback · 2017-07-31T23:21:26Z

doc/source/whatsnew/v0.21.0.txt

+  1  2.0  7.0
+  2  3.0  9.0>
+
+Both of these now raise a ``UserWarning`` about the potential for unexpected behavior. See `Attribute Access <https://pandas.pydata.org/pandas-docs/stable/indexing.html#attribute-access>`__.


use :ref:`Attribute Access <indexing.attribute_access>`

the section in indexing.rst

jreback · 2017-07-31T23:26:03Z

pandas/core/generic.py

        return result

    def _set_item(self, key, value):
+        if isinstance(key, str) and callable(getattr(self, key, None)):


use compat.string_types rather than str here

Using string_types causes failures in Python2 when creating columns that have unicode characters in their names. See c90aa22.

jreback · 2017-07-31T23:26:22Z

doc/source/whatsnew/v0.21.0.txt

   df['C'] = pd.to_numeric(df['C'], errors='coerce')
   df.dtypes

+.. _whatsnew_0210.enhancements.column-creation:


call this attribute_access (rather than column-creation)

As part of warning check, object type of potential attributes was checked for subtypes of pd.compat.str_types before being checked for overlap with methods defined on ndframes. This causes decode errors in Python2 when users attempt to add columns with unicode column names. Fix is to compare against `str`.

deniederhut · 2017-08-04T13:32:20Z

@jreback I think we might be ready for the final check

jreback · 2017-08-07T11:06:07Z

thanks @deniederhut nice changes!

jreback · 2017-08-08T13:12:36Z

@deniederhut
see https://travis-ci.org/MacPython/pandas-wheels/jobs/262048673

this is a 32-bit python 2.7 build; there are some warnings at the end. See if you can repro (you can try using a 64-bit python 2.7 build)

jorisvandenbossche · 2017-08-08T14:24:42Z

I have some general comments on this PR (sorry for being late with this feedback):

I like that you now get a warning when trying to create a new column with df.col_name = [..] (which silently fails at creating a new column), that's very useful! However, the case when the name you try to set is a builtin, eg df.mean = [...] doesn't raise a warning.
I think it would be nice to add a warning then as well (as it is actually a more harmful case, it is not only silently failing, but also overwriting your mean() method)
IMO the warning when setting a new column with df['new_col'] = .. which has the same name as a builtin method is a bit overkill. I understand it is confusing from time to time that the attribute access does not always work, but IMO there are many more people who use column names like mean without any problem that would now get warnings (one example that this is common is applying functions in groupby operations that create such warnings like s.groupby(..).agg(['mean', 'median'])). So I am -1 on this change.
See also comment above about df.mean = .. not raising a warning while df['mean] = .. does, but that's something else as it is actually overwriting the method (and this can also be easily fixed to raise I think).
Last, if we keep this warning, I think we should also do it for attributes and not only methods? (eg currently df['shape'] does not raise a warning)

jorisvandenbossche

and also one specific code comment

jorisvandenbossche · 2017-08-08T14:27:40Z

pandas/core/generic.py

            except (AttributeError, TypeError):
+                if isinstance(self, ABCDataFrame) and (is_list_like(value)):
+                    warnings.warn("Pandas doesn't allow Series to be assigned "
+                                  "into nonexistent columns - see "


I would write this a bit more generic, as it does not only raise for Series objects (and I also find it a bit confusing, as pandas certainly allows assigning Series objects, just with another syntax).

Maybe something like "pandas doesn't allow to add a new column using attribute access" ? (can certainly be improved further)

deniederhut · 2017-08-12T21:15:13Z

Happy to dig into this some more - two questions:

What is the consensus among the maintainers about warning on name collision?
Should this be submitted as a separate PR?

jreback · 2017-08-12T21:34:08Z

new PR

shoyer · 2017-08-12T22:04:01Z

I also just saw this PR. I agree with @jorisvandenbossche we should not raise warnings when setting column names that conflict with built-in methods. This would be a major source of annoyance for users.

I think we're pretty clear that attribute style access for columns is a convenience feature, not something to be relied on for arbitrary names. In contrast, we do guarantee that __getitem__ works for any valid column name.

jorisvandenbossche · 2017-08-16T13:12:54Z

@deniederhut do you want to do a PR for this?

@jreback what do you think about the concerns of @shoyer and me? Fine with changing this part of the PR back?

deniederhut · 2017-08-16T13:45:57Z

I'll take a look this weekend

…

On Wed, Aug 16, 2017, 08:13 Joris Van den Bossche ***@***.***> wrote: @deniederhut <https://github.com/deniederhut> do you want to do a PR for this? @jreback <https://github.com/jreback> what do you think about the concerns of @shoyer <https://github.com/shoyer> and me? Fine with changing this part of the PR back? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16951 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGjNrIJP7vd57k2d7VACiSiUdIES21mXks5sYurrgaJpZM4OZEMp> .

jorisvandenbossche · 2017-08-16T20:41:45Z

@deniederhut cool!
I also just opened #17268 to keep track of this

jreback reviewed Jul 15, 2017

View reviewed changes

jreback requested changes Jul 15, 2017

View reviewed changes

jreback added the Compat pandas objects compatability with Numpy or Python functions label Jul 15, 2017

jreback reviewed Jul 15, 2017

View reviewed changes

jreback requested changes Jul 15, 2017

View reviewed changes

jreback reviewed Jul 16, 2017

View reviewed changes

deniederhut force-pushed the fix/setting-into-attribute branch from 6216954 to 84f04d9 Compare July 19, 2017 01:18

jreback approved these changes Jul 19, 2017

View reviewed changes

jreback added the Error Reporting Incorrect or improved errors from pandas label Jul 19, 2017

deniederhut force-pushed the fix/setting-into-attribute branch from 06af1a1 to cf2abfd Compare July 19, 2017 12:09

deniederhut force-pushed the fix/setting-into-attribute branch 2 times, most recently from e7d7def to d8c1faa Compare July 29, 2017 16:48

jbrockmendel reviewed Jul 29, 2017

View reviewed changes

jreback mentioned this pull request Jul 29, 2017

Suggestion: Warning for DataFrame colname-methodname clash #5904

Closed

jreback requested changes Jul 31, 2017

View reviewed changes

deniederhut added 7 commits August 4, 2017 07:15

DOC: moves examples from whatsnew to indexing

f3fdd19

FIX: replaces ndim check with ABC subclass check

c6fe062

CLN: breaks line to avoid 81 char length

8ad4d05

TST: renames 'bool' columns in pytables test to avoid collision warning

e5cc166

BUG: fixes reversion of isnull/isna produced in merge

d38ecca

DOC: fixes references in indexing and whatsnew

b86546e

deniederhut force-pushed the fix/setting-into-attribute branch from d8c1faa to b86546e Compare August 4, 2017 12:16

jreback approved these changes Aug 7, 2017

View reviewed changes

jreback merged commit 7cc0fac into pandas-dev:master Aug 7, 2017

jorisvandenbossche reviewed Aug 8, 2017

View reviewed changes

jorisvandenbossche mentioned this pull request Aug 16, 2017

Remove warning when setting columns that conflict with built-in methods #17268

Closed

deniederhut mentioned this pull request Aug 20, 2017

BUG: revert collision warning #17298

Merged

4 tasks

alchenist mentioned this pull request Mar 21, 2018

UserWarning when initializing PandasDataSet with pandas >= 0.21.0, ge == 0.3.2 great-expectations/great_expectations#241

Closed

		assert isinstance(pd.Period('2012', freq='A-DEC'), gt.ABCPeriod)


		class TestABCWarnings(object):

Uh oh!

ENH: Add warning when setting into nonexistent attribute #16951

ENH: Add warning when setting into nonexistent attribute #16951

Uh oh!

Conversation

deniederhut commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on August 04, 2017 at 12:16 Hours UTC

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback commented Jul 15, 2017

Uh oh!

deniederhut commented Jul 15, 2017

Uh oh!

jreback commented Jul 15, 2017

Uh oh!

codecov bot commented Jul 15, 2017

Codecov Report

Uh oh!

codecov bot commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 16, 2017

Uh oh!

jreback commented Jul 16, 2017

Uh oh!

deniederhut commented Jul 19, 2017

Uh oh!

deniederhut commented Jul 15, 2017 •

edited

Loading

pep8speaks commented Jul 15, 2017 •

edited

Loading

jreback commented Jul 15, 2017 •

edited

Loading

codecov bot commented Jul 15, 2017 •

edited

Loading

jreback Jul 16, 2017 •

edited

Loading