Skip to content

BUG/TST: reset setitem_copy on object enlargement #5584

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

Merged
merged 4 commits into from
Nov 29, 2013

Conversation

jreback
Copy link
Contributor

@jreback jreback commented Nov 25, 2013

related #5581
closes #5597

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

@jtratner, cc @TomAugspurger

this fixes the warning in tests/test_index.py. I don't believe this changes any thing else.

can you give a whirl.

@TomAugspurger
Copy link
Contributor

Yep that did it.

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

@TomAugspurger pls give this updated one a whirl... can you post a script of what you are doing...I can add as tests...

@TomAugspurger
Copy link
Contributor

I'm just running the index.py test.

On master:

(pandas-dev) ~/E/p/l/p/s/p/p/tests (master↑ ) pwd
/Users/tom/Envs/pandas-dev/lib/python2.7/site-packages/pandas/pandas/tests
(pandas-dev) ~/E/p/l/p/s/p/p/tests (master↑ ) nosetests test_index.py
................................................................................................................................................................./Users/tom/Envs/pandas-dev/lib/python2.7/site-packages/pandas/pandas/core/generic.py:1029: SettingWithCopyWarning: A value is trying to be set on a copy of a slice from a DataFrame.
Try using .loc[row_index,col_indexer] = value instead
  warnings.warn(t, SettingWithCopyWarning)
.................
----------------------------------------------------------------------
Ran 178 tests in 1.131s

OK

This PR:

(pandas-dev) ~/E/p/l/p/s/p/p/tests (jreback-copy_warn=) nosetests test_index.py
..................................................................................................................................................................................
----------------------------------------------------------------------
Ran 178 tests in 0.928s

OK

The causing the warning was this one:

    def test_set_value_keeps_names(self):
        # motivating example from #3742
        lev1 = ['hans', 'hans', 'hans', 'grethe', 'grethe', 'grethe']
        lev2 = ['1', '2', '3'] * 2
        idx = pd.MultiIndex.from_arrays(
            [lev1, lev2],
            names=['Name', 'Number'])
        df = pd.DataFrame(
            np.random.randn(6, 4),
            columns=['one', 'two', 'three', 'four'],
            index=idx)
        df = df.sortlevel()
        self.assertEqual(df.index.names, ('Name', 'Number'))
        df = df.set_value(('grethe', '4'), 'one', 99.34)  # <<<< this line
        self.assertEqual(df.index.names, ('Name', 'Number'))

@jtratner
Copy link
Contributor

Why don't we default SettingWithCopy to error in the test suite? (and then
make sure the default is warning and test what happens with warning/None)

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

@TomAugspurger ok...that was my test case as well

@jtratner in theory you could do that after this PR. In test/test_indexing.py, the options are set (for testing), but other than that the default ('warn') is used.

Let me change it

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

@jtratner I can set it in a couple of the likely test suits...any way to change it 'globally' though? (only for testing)?

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

the right way to do this is to have a pandas unittest class then have all tests inherit from this, and have the setup in a sub always call this, but more work that what I am doing

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

too hacky?

I don't want to just import the test suite to actually trigger this so I think have to actually call it at the top of each file (because you could just call that file independently). We don't have a 'global' test suite setup ATM

@jtratner
Copy link
Contributor

Hmm, that's true.

@jtratner
Copy link
Contributor

Why not just define a class with a setUp method and have other modules
inherit that?

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

@jtratner you could do that

but then you have to make sure that an existing setUp calls it and change the inheriting from unittest every where

that would then allow say ignoring of FutureWarning and such in an easy way (but so will this)

@jtratner
Copy link
Contributor

Okay, however you want to do it is fine with me. Maybe there's a global
nose setup that you can do via environment variable?
On Nov 25, 2013 8:54 AM, "jreback" [email protected] wrote:

@jtratner https://github.com/jtratner you could do that

but then you have to make sure that an existing setUp calls it and change
the inheriting from unittest every where

that would then allow say ignoring of FutureWarning and such in an easy
way (but so will this)


Reply to this email directly or view it on GitHubhttps://github.com//pull/5584#issuecomment-29218769
.

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

looks like you can do setUpClass a class method (assuming you sub-class).... then wouldn't need to have the setUp actually call super...

@jreback
Copy link
Contributor Author

jreback commented Nov 25, 2013

@jtratner alright...revised to baseclass all TestCases....whoosh!

@jtratner
Copy link
Contributor

Okay I'll take a look tonight.

@jreback
Copy link
Contributor Author

jreback commented Nov 27, 2013

@jtratner give this a look see pls

TST: eliminate SettingWithCopyWarnings in tests (catch them)

TST: tests for GH5597

BUG: don't set copy on equal indexes after an operation
@jreback
Copy link
Contributor Author

jreback commented Nov 29, 2013

@jtratner ok with this?


"""

class TestNewOffsets(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this get deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was all commented out - afaik never used

@jtratner
Copy link
Contributor

@jreback this looks fine, though I realized we could have just defined a single setup_package and teardown_package in each of the testing modules instead:

So could have just done this:

def setup_package():
    pd.set_option('chained_assignment','raise')
    #print("setting up: {0}".format(cls))

def teardown_package():
    #print("tearing down up: {0}".format(cls))
    pass

and then in test __init__.py files just do

from pandas.util.testing import setup_package, teardown_package

so knowledge for the future.

@jreback
Copy link
Contributor Author

jreback commented Nov 29, 2013

ok that's prob about the same amount of code I guess
good 2 know though

jreback added a commit that referenced this pull request Nov 29, 2013
BUG/TST: reset setitem_copy on object enlargement
@jreback jreback merged commit d9b3340 into pandas-dev:master Nov 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spurious SettingWithCopyWarning
3 participants