BUG: align with broadcast_axis, #13194 #13198

pfrcks · 2016-05-17T09:46:32Z

Implemented the easy fix
Future work : To implement broadcast axis remaining functionality.
All tests in test_generic.py passed.

jreback · 2016-05-17T09:47:55Z

tests

we r on 0.18.2

pfrcks · 2016-05-17T09:50:39Z

Hi,

I seem to have made the documentation in the wrong file. I will correct it and push the changes.
Rest all is OK, right?
This is my first pull request to pandas, sorry for the noob errors.

Also, have I missed something?

pfrcks · 2016-05-17T09:55:55Z

@jreback I have corrected the documentation to the proper version file. Is this everything?
I have ran the tests also.
Am I missing something?

jreback · 2016-05-17T10:05:44Z

you need to add tests which fail before this patch and pass after

kdebrab · 2016-05-17T10:39:31Z

Apart from the indispensable unit test, I'd propose to move the lines:

if axis is not None:
    axis = self._get_axis_number(axis)

to right before the if broadcast_axis == 1 block, such that broadcasting will work (correctly) also in case align is called with axis='index' instead of axis=0.

Thanks!

codecov-io · 2016-05-17T10:57:35Z

Current coverage is 84.11% (diff: 100%)

Merging #13198 into master will not change coverage

@@             master     #13198   diff @@
==========================================
  Files           138        138          
  Lines         50388      50388          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          42386      42386          
  Misses         8002       8002          
  Partials          0          0

Powered by Codecov. Last update 4e4a7d9...1b5bbde

pfrcks · 2016-05-17T11:05:45Z

@jreback @kdebrab I've included the unit test and have also moved the location of the axis check.
Can you please take a look?

jreback · 2016-05-17T13:18:55Z

pandas/tests/test_generic.py

@@ -1874,6 +1874,52 @@ def test_pipe_panel(self):
        with tm.assertRaises(ValueError):
            result = wp.pipe((f, 'y'), x=1, y=1)

+    def test_align_joins(self):
+        df = DataFrame(np.array([[1., 2.], [3., 4.]]), columns=list('AB'))


add the issue number as a comment here

rename to test_align_broadcast_axis

pfrcks · 2016-05-17T14:48:32Z

@jreback Have made the required changes.
Please comment if something's lacking.

sinhrks · 2016-05-19T20:13:46Z

pandas/tests/frame/test_axis_select_reindex.py

+
+        # For 'inner' join
+        result = df.align(ts, join='inner', axis=0, broadcast_axis=1)
+        result1 = DataFrame(result[0])


Not needed. Test returned obj (result[0] and [1]) as it is.

@sinhrks I'm not sure that I understand. Can you please elaborate?

Pls remove DataFrame(result..) like below.

result = df.align(...) expected1 = ... expected2 = ... assert_frame_equal(result[0], expected1) assert_frame_equal(result[1], expected2)

sinhrks · 2016-05-19T20:21:22Z

Can u add tests with:

alternately different Index to clheck left and right join.
Series.align(DataFrame)

pfrcks · 2016-05-19T20:59:32Z

@sinhrks Sure. It's late night here right now. I will do this in the morning.

pfrcks · 2016-05-20T04:39:51Z

@sinhrks I've made some changes. Please take a look and comment. This is what you meant right?

jreback · 2016-05-20T12:50:22Z

doc/source/whatsnew/v0.18.2.txt

@@ -149,3 +149,4 @@ Bug Fixes
 - Bug in ``NaT`` - ``Period`` raises ``AttributeError`` (:issue:`13071`)
 - Bug in ``Period`` addition raises ``TypeError`` if ``Period`` is on right hand side (:issue:`13069`)
 - Bug in ``pd.set_eng_float_format()`` that would prevent NaN's from formatting (:issue:`11981`)
+- Fixed the bug in ``DataFrame.align()`` which was giving wrong output when supplied with the ``join`` argument. Earlier, upon supplying value of join argument as any of the four('outer', 'inner', 'left', 'right' ), align() was giving erraneous output. Align with broadcast_axis specified was using 'inner' join consistently irrespective of the value of 'join' provided when aligning dataframe and series on other axis. The problem was identified to be in pandas/core/generic.py and has been subsequently fixed. (:issue:`13194`) 


Single line of comments pls. these are read by the users and don't need this kind of explanation.

pfrcks · 2016-05-20T14:25:49Z

@jreback Done. Kindly check and comment.

jreback · 2016-05-20T14:37:35Z

pandas/tests/frame/test_axis_select_reindex.py

+        df = DataFrame(np.array([[1., 2.], [3., 4.]]), columns=list('AB'))
+        ts = Series([5., 6., 7.])
+
+        result = df.align(ts, join='right', axis=0, broadcast_axis=1)


try one of these with broadcast_axis='columns' and one with broadcast_axis='index' as well

left, right = df.align(....) and same below

jreback · 2016-05-20T14:37:55Z

pandas/tests/frame/test_axis_select_reindex.py

+        ts = Series([5., 6., 7.])
+
+        result = df.align(ts, join='right', axis=0, broadcast_axis=1)
+        expected1 = DataFrame(np.array([[1., 2.], [3., 4.],


call these expected_left and expected_right (and same below)

pfrcks · 2016-05-20T15:44:45Z

@jreback

    right = df.align(ts, join='right', axis=1, broadcast_axis='index')
    expected1_right = DataFrame([[pd.np.nan, pd.np.nan, pd.np.nan],
                                 [pd.np.nan, pd.np.nan, pd.np,nan]],
                                columns=[0, 1, 2])
    expected2_right = Series([5., 6., 7.])
    assert_frame_equal(right[0], expected1_right)
    assert_series_equal(right[1], expected2_right)

While trying to run the above code, nosetests gives the error

AssertionError: Series.index are different

    Series.index classes are not equivalent
    [left]:  Index([u'A', u'B'], dtype='object')
    [right]: RangeIndex(start=0, stop=2, step=1)

However, upon running in the interpreter

right = df.align(ts, join='right', axis=1, broadcast_axis='index')
right[1]
0 5.0
1 6.0
2 7.0
dtype: float64
right[1].index
RangeIndex(start=0, stop=3, step=1)
expected2_right = Series([5., 6., 7.])
expected2_right.index
RangeIndex(start=0, stop=3, step=1)

We find that both indexes are same.
I am currently in pandas_dev environment.
Is this something assert_series_frame is doing?

sinhrks · 2016-05-20T22:12:24Z

pandas/tests/series/test_indexing.py

+        self.assertTrue(result[1].equals(expected2))
+
+        # Series.align(DataFrame) tests, 'inner' join
+        result = ts.align(df, join='inner', axis=0, broadcast_axis=1)


we can make all tests more comprehensive as "inner" and "outer" outputs the same, and "left" and "right" outputs inversed results.

result = df.align(series, ...) self.assert_frame_equal(result[0], expected1) self.assert_frame_equal(result[1], expected2) result = series.align(df, ...) self.assert_frame_equal(result[0], expected2) self.assert_frame_equal(result[1], expected1)

missed @jreback suggestion for variable names:

left, right = df.align(series, ...) self.assert_frame_equal(left, expected_left) self.assert_frame_equal(right, expected_right) left, right = series.align(df, ...) self.assert_frame_equal(right, expected_left) self.assert_frame_equal(left, expected_right)

@jreback Can you please look into the issue I have reported above.

a single Ping is enough then you need to wait for comments

jreback · 2016-09-09T22:43:59Z

can you rebase / update?

pfrcks · 2016-09-11T12:38:32Z

@jreback I'm not sure I understand. I have rebased my_branch with master.

jreback · 2016-09-11T12:41:50Z

doesn't look like it
you need to push

pfrcks · 2016-09-11T13:07:50Z

@jreback
I have used the following commands:

git checkout my_branch
git rebase master
git push -u origin my_branch

pfrcks · 2016-09-11T13:08:26Z

Sorry I closed my mistake

jreback · 2016-09-11T13:17:54Z

got rebase origin/master

jreback · 2016-09-11T13:18:07Z

got -> git

jreback · 2016-09-11T13:18:24Z

you are rebasing to a local master which is not updated

pfrcks · 2016-09-11T13:38:33Z

@jreback

I followed the steps:

git remote add ups https://github.com/pydata/pandas.git
git fetch ups
git checkout master
git rebase ups/master

First, rewinding head to replay your work on top of it...
Applying: BUG: GH13194 fixed
Applying: DOC : Change in release notes
Using index info to reconstruct a base tree...
A doc/source/whatsnew/v0.18.2.txt
:19: trailing whitespace.

Fixed the bug in DataFrame.align() which was giving wrong output when supplied with the join argument. (:issue:13194)
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): doc/source/whatsnew/v0.18.2.txt deleted in HEAD and modified in DOC : Change in release notes. Version DOC : Change in release notes of doc/source/whatsnew/v0.18.2.txt left in tree.
Failed to merge in the changes.
Patch failed at 0002 DOC : Change in release notes
The copy of the patch that failed is found in:
/home/pfrcks/issues/pandas-pfrcks/.git/rebase-apply/patch
When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

I removed the whitespace error and then synced the changes but still I am getting the same issue.

jreback · 2016-11-22T11:46:35Z

closing, but if you want to update / rebase. pls reopen

BUG: GH13194 fixed

b4867df

DOC : Change in release notes

a4e6804

TST: Added tests for pandas-dev#13194

51c5968

CLN: Changed location of axis check

e15e17e

jreback changed the title ~~BUG: GH13194 fixed~~ BUG: align with broadcast_axis, #13194 May 17, 2016

jreback added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels May 17, 2016

jreback reviewed May 17, 2016
View reviewed changes

TST : Renamed and changed the location of unittests.

e6ec1d9

DOC : improved the documentation in release notes

1b5bbde

sinhrks reviewed May 19, 2016
View reviewed changes

TST: Added and modified unittests

fb1b41f

jreback reviewed May 20, 2016
View reviewed changes

TST: Restructured and moved unittests

248e8da

jreback reviewed May 20, 2016
View reviewed changes

sinhrks reviewed May 20, 2016
View reviewed changes

pfrcks closed this Sep 11, 2016

pfrcks reopened this Sep 11, 2016

Cleaned up doc

6de9b0c

jreback closed this Nov 22, 2016

BUG: align with broadcast_axis, #13194 #13198

BUG: align with broadcast_axis, #13194 #13198

Conversation

pfrcks commented May 17, 2016

jreback commented May 17, 2016

pfrcks commented May 17, 2016 • edited Loading

pfrcks commented May 17, 2016

jreback commented May 17, 2016

kdebrab commented May 17, 2016 • edited Loading

codecov-io commented May 17, 2016 • edited Loading

Current coverage is 84.11% (diff: 100%)

pfrcks commented May 17, 2016

Choose a reason for hiding this comment

pfrcks commented May 17, 2016

sinhrks May 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhrks commented May 19, 2016

pfrcks commented May 19, 2016

pfrcks commented May 20, 2016

Choose a reason for hiding this comment

pfrcks commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback May 20, 2016 • edited Loading

Choose a reason for hiding this comment

pfrcks commented May 20, 2016

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

jreback commented Sep 9, 2016

pfrcks commented Sep 11, 2016

jreback commented Sep 11, 2016

pfrcks commented Sep 11, 2016

pfrcks commented Sep 11, 2016

jreback commented Sep 11, 2016

jreback commented Sep 11, 2016

jreback commented Sep 11, 2016

pfrcks commented Sep 11, 2016

jreback commented Nov 22, 2016

pfrcks commented May 17, 2016 •

edited

Loading

kdebrab commented May 17, 2016 •

edited

Loading

codecov-io commented May 17, 2016 •

edited

Loading

sinhrks May 19, 2016 •

edited

Loading

jreback May 20, 2016 •

edited

Loading