Adding depth-wise table stacking by geekypathak21 · Pull Request #8939 · astropy/astropy

geekypathak21 · 2019-06-29T20:00:47Z

Trying to fixing:#6660
Not ready for a final review but contains an implementation of cstack method for depth wise stacking.

taldcroft · 2019-06-30T11:31:41Z

@himanshupathak21061998 - thanks, looks like a good start. Indeed cstack is quite similar to vstack, and going back #6660 the crux was about new_like. Your implementation ends up passing a tuple as length, which happens to work for Column, but in general length is currently required by API to be an int. For instance if you try to cstack with Time objects it fails. So either we need to do something with new_like, or something else...

Looking at this I got an idea to make this implementation super simple. Roughly speaking, cstack is just exactly vstack, but with the final output columns reshaped correctly. So I think it might work to literally call vstack, get that table, and then reshape and transpose the output columns.

One hint, which is a brand new feature of table. You can avoid making a new output table object with the following:

    # check tables all same length
    out = vstack(tables, join_type, metadata_conflicts)

    for name, col in out.columns.items():
        new_col = reshaped_and_transposed(col)
        out.columns.__setitem__(name, new_col, validated=True)

This temporarily corrupts out by having different length columns, but it all works out in the end. Maybe worth an explicit check of that in the code for sanity.

The reshape and transpose is simple for input 1-d columns, but getting it straight for input N-d columns is a little trick and that is a challenge left for you! 😄

geekypathak21 · 2019-07-02T19:20:21Z

Thanks for helping @taldcroft and sorry for the delay. Changes updated now I will start writing tests for this.

bsipocz · 2019-07-02T19:24:57Z

@himanshupathak21061998 - if you rebase, the 32bit errors would go away.

taldcroft · 2019-07-03T17:21:35Z

@himanshupathak21061998 - I think the whole thing can be done far more simply by fully-using the similarity of vstack and cstack. I thought you might get this independently, but here is a solution that seems to work:

def cstack(tables, join_type='outer', metadata_conflicts='warn'):
    n_rows = set(len(table) for table in tables)
    if len(n_rows) != 1:
        raise ValueError()
    n_row = n_rows.pop()

    out = vstack(tables, join_type, metadata_conflicts)

    for name, col in out.columns.items():
        col = out[name]

        # Reshape to so each original column is now in a row.
        # If entries are not 0-dim then those additional shape dims
        # are just carried along.
        # [x x x y y y] => [[x x x],
        #                   [y y y]]
        col.shape = (len(tables), n_row) + col.shape[1:]

        # Transpose the table and row axes to get to
        # [[x, y],
        #  [x, y]
        #  [x, y]]
        axes = np.arange(len(col.shape))
        axes[:2] = [1, 0]

        # This temporarily makes `out` be corrupted (columns of different
        # length) but it all works out in the end.
        out.columns.__setitem__(name, col.transpose(axes), validated=True)

    return out

BTW, no need to follow the legacy pattern of having an internal _stack method. That is from the ancient days when we supported stacking numpy structured arrays.

taldcroft · 2019-07-03T17:26:00Z

Example:

In [15]: t = simple_table()

In [16]: t2 = simple_table()

In [17]: c = cstack([t, t2])

In [18]: c2 = cstack([t, t2])

In [19]: d = cstack([c, c2])

In [20]: d['a'].shape
Out[20]: (3, 2, 2)

In [21]: c['a'].shape
Out[21]: (3, 2)

In [22]: t['a'].shape
Out[22]: (3,)

In [23]: d['a'][0]
Out[23]: 
array([[1, 1],
       [1, 1]])

In [24]: e = cstack([t, c])
---------------------------------------------------------------------------
TableMergeError                           Traceback (most recent call last)
<ipython-input-24-71e13d610856> in <module>()
----> 1 e = cstack([t, c])
...
TableMergeError: columns have different shapes

geekypathak21 · 2019-07-03T19:30:06Z

In [20]: d['a'].shape
Out[20]: (3, 2, 2)

Sorry, but confusion was because of this case I thought it be like this

d['a'].shape
Out[4]: (3, 4)

My mistake I thought that shape with more than two elements tuple is not allowed. Next time I will try better thanks @taldcroft

geekypathak21 · 2019-07-05T20:37:00Z

+
+        with pytest.raises(TableMergeError):
+            table.cstack([t2, t3])
+


Is it okay that we cannot merge column having a different shape for cstack?

Yes. This sort of stacking depends on the input column shapes being the same. The same happens for numpy vstack:

In [16]: a = np.array([[1, 2]]) In [17]: b = np.array([[3, 4]]) In [18]: np.vstack([a, b]) Out[18]: array([[1, 2], [3, 4]]) In [19]: b = np.array([[[3, 33], [4, 44]]]) In [20]: np.vstack([a, b]) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) ~/git/astropy/cstack.py in <module>() ----> 1 np.vstack([a, b]) ~/miniconda3/envs/astropy/lib/python3.6/site-packages/numpy/core/shape_base.py in vstack(tup) 232 233 """ --> 234 return _nx.concatenate([atleast_2d(_m) for _m in tup], 0) 235 236 def hstack(tup): ValueError: all the input arrays must have same number of dimensions

geekypathak21 · 2019-07-05T20:42:00Z

+      1 .. 5 3 .. 7
+      2 .. 6 4 .. 8
+    """
+    n_rows = set(len(table) for table in tables)


If we enter cstack([t1, t1[1]]) it returns true should I change this behaviour. I thought it is better to ask before doing?

I need to think about this.

geekypathak21 · 2019-07-05T20:47:29Z

@taldcroft thanks for helping. I have performed all changes as required.

taldcroft

Looking good, just a few things to change.

taldcroft · 2019-07-06T10:29:20Z

+    """
+    n_rows = set(len(table) for table in tables)
+    if len(n_rows) != 1:
+        raise ValueError('Tables length are not matching. '


Use 'table lengths must all match for cstack.'.

taldcroft · 2019-07-06T10:58:32Z

+        self._setup(operation_table_type)
+        t2 = self.t1.copy()
+        out = table.cstack([self.t1, t2['a']])
+        assert out.pformat() == ['  a [2]      b [2]  ',


Since the repr for a multi-dimensional column does not show all the data, you need to compare with a full version of each column. A test helper function like this might help:

def compare_cstack(tables, out): for ii, tbl in enumerate(tables): for name, out_col in out.columns.items(): if name in tbl.colnames: assert np.all(tbl[name] == out[name][:, ii]) else: assert np.all(out[name].mask[:, ii])

With this what is important is that every input data value is different. So in the setup just use the simplest values and keep incrementing, e.g.:

self.t1 = t_cls.read([' a b', ' 0. a', ' 1. b'], format='ascii') self.t2 = t_cls.read([' a b c', ' 2. c 4', ' 3. d 5'], format='ascii') self.t3 = t_cls({'a': [[7., 8.], [9., 10.]], 'b': [['e', 'f'], ['g', 'h']]}, names=('a', 'b')) # and so on.

taldcroft · 2019-07-06T11:05:56Z

+                         'b': [['foo', 'bar'], ['pez', 'sez']]},
+                        names=('a', 'b'))
+        self.t4 = t_cls(self.t1, copy=True, masked=t_cls is Table)
+        self.t5 = t_cls({'a': [[4., 2.], [1., 6.]],


One of the inputs should have a masked column with a masked element.

taldcroft · 2019-07-06T11:07:10Z

+            t1 = self.t1
+            t2 = self.t2
+            t4 = self.t4
+            t12 = table.cstack([t1, t2], join_type='outer')


The t12 and t124 tests are basically the same and not improving coverage. So just keep the t124 test.

taldcroft · 2019-07-06T11:07:31Z

+        t2 = self.t2
+        t4 = self.t4
+
+        t12 = table.cstack([t1, t2], join_type='inner')


Same comment as before, delete the t12 test.

taldcroft · 2019-07-06T11:12:37Z

+
+        with pytest.raises(TableMergeError):
+            table.cstack([t2, t3])
+


Yes. This sort of stacking depends on the input column shapes being the same. The same happens for numpy vstack:

In [16]: a = np.array([[1, 2]]) In [17]: b = np.array([[3, 4]]) In [18]: np.vstack([a, b]) Out[18]: array([[1, 2], [3, 4]]) In [19]: b = np.array([[[3, 33], [4, 44]]]) In [20]: np.vstack([a, b]) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) ~/git/astropy/cstack.py in <module>() ----> 1 np.vstack([a, b]) ~/miniconda3/envs/astropy/lib/python3.6/site-packages/numpy/core/shape_base.py in vstack(tup) 232 233 """ --> 234 return _nx.concatenate([atleast_2d(_m) for _m in tup], 0) 235 236 def hstack(tup): ValueError: all the input arrays must have same number of dimensions

geekypathak21 · 2019-07-10T21:46:08Z

@taldcroft Do I need to include more tests for it.

taldcroft · 2019-08-09T22:25:33Z

@himanshupathak21061998 - I did final review and added a few commits to try to finish this up. Team effort!

bsipocz · 2019-08-09T22:53:36Z

@taldcroft - I suppose a rebase would solve at least some of the CI issues.

taldcroft · 2019-08-10T10:30:54Z

@bsipocz - I rebased on latest master but it is still failing to build in Travis. Any clue?

bsipocz · 2019-08-10T23:52:39Z

I see the same issue on master, but don't yet see the cause for it. It's definitely unrelated to this PR.

bsipocz · 2019-08-12T16:57:46Z

@taldcroft - Travis is all green now.

pllim added table Enhancement labels Jun 29, 2019

pllim added this to the v4.0 milestone Jun 29, 2019

pllim requested a review from taldcroft June 29, 2019 20:04

dhomeier mentioned this pull request Jul 1, 2019

Deprecation warnings and test failures under Python 3.8 #8938

Merged

geekypathak21 force-pushed the add-cstack branch from 006ae36 to 27efe0e Compare July 2, 2019 19:29

geekypathak21 force-pushed the add-cstack branch 2 times, most recently from 27efe0e to ec7243e Compare July 5, 2019 20:31

geekypathak21 commented Jul 5, 2019

View reviewed changes

taldcroft requested changes Jul 6, 2019

View reviewed changes

geekypathak21 force-pushed the add-cstack branch from 63fb8ac to db14e6b Compare July 10, 2019 23:15

geekypathak21 and others added 6 commits August 10, 2019 05:56

cstack table operation implemented

9bb9b53

Changes updated in cstack()

dd8c218

Change log update adding test and update changes in cstack()

1a97417

Handle one input object (table, column, etc)

ba30e2e

Minor improvements to test coverage

32c1085

Add narrative docs

20ba31f

taldcroft force-pushed the add-cstack branch from 2a15d75 to 20ba31f Compare August 10, 2019 09:58

taldcroft approved these changes Aug 12, 2019

View reviewed changes

taldcroft merged commit 50775e2 into astropy:master Aug 12, 2019

bsipocz mentioned this pull request Aug 12, 2019

Add depth-wise table stacking (dstack?) #6660

Closed

Conversation

geekypathak21 commented Jun 29, 2019

Uh oh!

taldcroft commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geekypathak21 commented Jul 2, 2019

Uh oh!

bsipocz commented Jul 2, 2019

Uh oh!

taldcroft commented Jul 3, 2019

Uh oh!

taldcroft commented Jul 3, 2019

Uh oh!

geekypathak21 commented Jul 3, 2019

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!

geekypathak21 commented Jul 5, 2019

Uh oh!

taldcroft left a comment

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!

geekypathak21 commented Jul 10, 2019

Uh oh!

taldcroft commented Aug 9, 2019

Uh oh!

bsipocz commented Aug 9, 2019

Uh oh!

taldcroft commented Aug 10, 2019

Uh oh!

bsipocz commented Aug 10, 2019

Uh oh!

bsipocz commented Aug 12, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taldcroft commented Jun 30, 2019 •

edited

Loading