Support join, hstack, vstack for Quantity by taldcroft · Pull Request #5841 · astropy/astropy

taldcroft · 2017-02-24T21:30:05Z

This is a different approach to #5811 that does two things:

Specifically supports join, hstack, and vstack for Quantity mixin columns
Introduces the new empty_like mixin protocol method for more generally improving high-level operations support for mixin columns like Time and SkyCoord.

For vstack and inner join with Time and SkyCoord, what we need is analogous empty_like methods which take a list of objects and make an empty container that is consistent with the inputs. Then of course we need to be able to set elements in the new empty container. Details TBD in a future PR.

cc: @mhvk

taldcroft · 2017-02-24T21:31:34Z

        _col_name_map.update(col_name_map)

+    # Merge column and table metadata
+    _merge_col_meta(out, arrays, col_name_map, metadata_conflicts=metadata_conflicts)


This doesn't strictly need to be inside _vstack yet, but eventually empty_like will merge other meta attributes like description, so it that will need to have metadata_conflicts.

taldcroft · 2017-02-24T21:32:13Z

    return newcol


+def _merge_ndarray_like_cols(cols):


Not clear exactly where this should live, but for now in column.py works.

taldcroft · 2017-02-24T21:33:00Z

+    def empty_like(cls, cols, **kwargs):
+        from ..table.column import Column
+        length = kwargs['length']
+        return cls(Column.empty_like(cols, length=length))


Using Column like this is a quick hack.

taldcroft · 2017-02-26T17:51:22Z

@mhvk - I did a wholesale copy of the excellent test_operations.py and conftest.py changes you made for #5811. Sorry you won't get commit credit for all that work if this PR ends up being merged.

taldcroft · 2017-02-26T17:54:43Z

The name empty_like is obviously intended to invoke some familiarity from the numpy method, but I can imagine that this could be confusing because it is a bit different. So I would be happy to change the name.

taldcroft · 2017-02-26T17:58:51Z

-                                     right[right_name].take(right_out))
+            cols = [left[left_name], right[right_name]]
+            col_cls = _get_out_class(cols)
+            out[out_name] = col_cls.empty_like(cols, n_out, metadata_conflicts, out_name)


To do: check for no empty_like method and raise a helpful exception if not.

taldcroft · 2017-02-26T17:59:12Z


+def _merge_ndarray_like_cols(cols, metadata_conflicts, name, attrs):
+    """
+    Empty like


To do: useful docstring.

taldcroft · 2017-02-26T18:00:22Z

 __all__ = ['join', 'hstack', 'vstack', 'unique']


-def _merge_col_meta(out, tables, col_name_map, idx_left=0, idx_right=1,


This is now handled "locally" from within empty_like.

taldcroft · 2017-02-26T18:01:59Z

+        if issubclass(obj.__class__, out_class):
+            out_class = obj.__class__
+
+    if any(not issubclass(out_class, obj.__class__) for obj in objs):


This now adds an explicit check that all the input objects have consistent inheritance.

taldcroft · 2017-02-26T18:04:24Z

+
+        col_cls = _get_out_class(cols)
+        try:
+            out[out_name] = col_cls.empty_like(cols, n_rows, metadata_conflicts, out_name)


To do: I think it is more clear to check for an empty_like method here instead of pre-checking above.

taldcroft · 2017-02-26T18:06:15Z

        left_name, right_name = col_name_map[out_name]
        if left_name and right_name:  # this is a key which comes from left and right
-            out[out_name] = out.ColumnClass(length=n_out, name=out_name, dtype=dtype, shape=shape)
-            out[out_name] = np.where(right_mask,


This was a latent bug: the previous statement (setting out[out_name] = out.ColumnClass(..) was actually being entirely ignored because this statement now makes a completely new column instead of in-place assignment. Oops.

Indeed, nice to have solved that!

taldcroft · 2017-02-26T18:41:09Z

This is ready for review.

mhvk · 2017-02-26T21:02:19Z

@taldcroft - my day job is catching up with me, so I had only a brief look so far. But enough to know that I like the idea of letting the classes create an empty instance of themselves very much! It seems an obviously superior approach over mine, as it should be relatively easy to extend to Time and SkyCoord, etc., and keeps the knowledge of how mixin columns behave where it should be, in the columns themselves.

My main comment so far is that I think this is even better done on top of info. In part simply because that keeps all table-related stuff together (and thus makes instructions on how to become table-compatible clearer), but also because it seems something quite similar is already being done by the _construct_from_dict method you added to info for the yaml work. If so, the _get_out_class should be adapted.

p.s. I think I also like the idea of having an empty_like best, but obviously, info could also gain a concatenate method; indeed, that would seem sufficiently generically useful, that I wouldn't mind having that on the classes itself... (Of course, Time and SkyCoord allow the simplest form of concatenation already via the constructor.) But one of course does not quite exclude the other; maybe start with working via info and we can always move it down.

taldcroft · 2017-02-26T23:11:42Z

@mhvk - thanks for the quick look, so I'll run with this and get it into final review shape.

taldcroft · 2017-05-08T19:53:29Z

@mhvk - I think this is ready for another look. I suggest looking at the last 6 commits one by one to see what happened.

I'm thinking next about Time - adding __setitem__ and TimeInfo.empty_like. More on that later.

mhvk

This looks very nice indeed! It seem all I could come up with was utter nitpicks.

mhvk · 2017-05-09T00:50:11Z


 class FalseArray(np.ndarray):
+    """
+    Class to create a stub ``mask`` property which is a boolean array of


It would be nice for the docstring to have the usual title and then description, i.e.,

"""Always unmasked array for use in columns without a proper mask. Description...

Same for other classes. Also, if I recall correctly, periods at end of descriptions of parameters actually matter...

Done (I think). Not sure what you mean about "periods at end of descriptions of parameters actually matter". I did remove one period in a parameter description so they are all consistent (with no period). I also noticed that sphinx takes the first full sentence as the method description in the summary table, so that was kind of working. But yes, I got a little sloppy.

mhvk · 2017-05-09T00:54:13Z

-    `tables` is a list of at least one element and that they are all
-    Table (subclass) instances.  This doesn't handle complicated
+    From a list of input objects ``objs`` get merged output object class.
+    This is just taken as the deepest subclass. This doesn't handle complicated


Again, add empty line here to split title from description (though I guess it doesn't matter much for a private function)

mhvk · 2017-05-09T00:54:23Z

+        if issubclass(obj.__class__, out_class):
+            out_class = obj.__class__
+
+    if any(not issubclass(out_class, obj.__class__) for obj in objs):


mhvk · 2017-05-09T00:55:45Z

    col_name_map = OrderedDict()

-    out = _vstack(tables, join_type, col_name_map)
+    out = _vstack(tables, join_type, col_name_map, metadata_conflicts)


It is not important, but to me it has become a bit unobvious why there is this private _vstack method -- I think this made more sense when that method dealt with just the recarray, but now that one passes in the tables, there is an argument for just sticking it in here.

mhvk · 2017-05-09T00:56:13Z

        left_name, right_name = col_name_map[out_name]
        if left_name and right_name:  # this is a key which comes from left and right
-            out[out_name] = out.ColumnClass(length=n_out, name=out_name, dtype=dtype, shape=shape)
-            out[out_name] = np.where(right_mask,


Indeed, nice to have solved that!

mhvk · 2017-05-09T01:02:12Z

+        # Make an empty quantity filled with Nan using the unit of the last one.
+        shape = (length,) + attrs.pop('shape')
+        dtype = attrs.pop('dtype')
+        data = np.empty(shape=shape, dtype=dtype)


It is needed to fill it with nan? Or are you setting up for having that mean masked? In any case, a short-cut is

data = np.full(shape, np.nan, dtype=dtype)

This was a left-over from an idea of always returning a "masked" object. But in the end I didn't go there, so this is removed.

mhvk · 2017-05-09T01:06:30Z

        value = map.pop('value')
        return self._parent_cls(value, **map)

+    def empty_like(self, cols, length, metadata_conflicts='warn', name=None):


I very much like the concept, but am not quite sold on the name, as np.empty_like works differently. Maybe shaped_like? Or new_like? (not a big deal...)

+1 to rename since different than np.empty_like

Fair enough, so will go with new_like.

mhvk · 2017-05-09T01:13:54Z

+            return out
+
+        def getattrs(col):
+            return {attr: getattr(col.info, attr) for attr in attrs


The double getattr makes this one-liner a bit less pleasant. Though I can also see why you didn't spell it out...

def getattrs(col): out = {} for attr in attrs: value = getattr(col.info, attr, None): if values is not None: out[attr] = value return out

I think the clarity provided by the functional expression is worth the double getattr.

mhvk · 2017-05-09T01:15:21Z

   * - :ref:`stack-vertically`
-     - Not implemented yet, pending definition of generic concatenation protocol
+     - Available for `~astropy.units.Quantity` and any other mixin classes that provide an
+       `empty_like() method`_ in the ``info`` descriptor.


Either here or below (probably better below), it should also be noted that the class has to allow setting elements (though I guess it is somewhat obvious...)

mhvk · 2017-05-09T01:19:14Z

+        t2 = self.t2
+        t4 = self.t4
+
+        # Key col 'a', should last value ('km')


comment sentence incomplete.

Oops, missed this one but will let it slide.

taldcroft

Comments addressed, thanks!

taldcroft · 2017-05-09T10:10:07Z

    col_name_map = OrderedDict()

-    out = _vstack(tables, join_type, col_name_map)
+    out = _vstack(tables, join_type, col_name_map, metadata_conflicts)


taldcroft · 2017-05-09T10:20:31Z

+        # Make an empty quantity filled with Nan using the unit of the last one.
+        shape = (length,) + attrs.pop('shape')
+        dtype = attrs.pop('dtype')
+        data = np.empty(shape=shape, dtype=dtype)


This was a left-over from an idea of always returning a "masked" object. But in the end I didn't go there, so this is removed.

taldcroft · 2017-05-09T10:21:47Z

        value = map.pop('value')
        return self._parent_cls(value, **map)

+    def empty_like(self, cols, length, metadata_conflicts='warn', name=None):


Fair enough, so will go with new_like.

taldcroft · 2017-05-09T10:38:34Z

   * - :ref:`stack-vertically`
-     - Not implemented yet, pending definition of generic concatenation protocol
+     - Available for `~astropy.units.Quantity` and any other mixin classes that provide an
+       `empty_like() method`_ in the ``info`` descriptor.


taldcroft · 2017-05-09T10:42:50Z


 class FalseArray(np.ndarray):
+    """
+    Class to create a stub ``mask`` property which is a boolean array of


Done (I think). Not sure what you mean about "periods at end of descriptions of parameters actually matter". I did remove one period in a parameter description so they are all consistent (with no period). I also noticed that sphinx takes the first full sentence as the method description in the summary table, so that was kind of working. But yes, I got a little sloppy.

taldcroft

Comments addressed, thanks!

mhvk · 2017-05-09T13:43:36Z

Not completely sure what's up with coverage but since this is about the best-tested part of astropy, I'm not going to worry about it. So, merging...

astrobot · 2017-05-09T13:44:01Z

@mhvk - thanks for merging this! However, I noticed the following issue with this pull request:

Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix this? Thanks!

If you believe the above to be incorrect (which I - @astrobot - very much doubt) you can ping @astrofrog

mhvk · 2017-05-09T13:45:09Z

Darn, did we really forget the change log?! Could you make a quick follow-up PR? Sorry for not noticing that...

bsipocz · 2017-05-09T14:04:31Z

Ohh, good hack-day project to finally set up astrobot to check for it while the PR is open...

mhvk · 2017-05-09T14:10:00Z

Yes, it could be another test required for merging...

Update changelog for #5841

pllim added table units Work in progress labels Feb 24, 2017

taldcroft commented Feb 24, 2017

View reviewed changes

taldcroft changed the title ~~WIP: support vstack for Quantity~~ Support join, hstack, vstack for Quantity Feb 26, 2017

taldcroft added Affects-release and removed Work in progress labels Feb 26, 2017

taldcroft added this to the v2.0.0 milestone Feb 26, 2017

taldcroft commented Feb 26, 2017

View reviewed changes

taldcroft mentioned this pull request Feb 26, 2017

Clean up table operations code #5843

Open

taldcroft force-pushed the table-mixin-ops branch from 9e95865 to 7369347 Compare May 8, 2017 17:35

taldcroft added 9 commits May 8, 2017 13:42

Generalize _get_out_class function

deae778

Move meta merging into _vstack (for now)

a807610

Create empty column instead of numpy arrays in vstack

2ed2b95

Minimal working vstack of Quantity [skip ci]

4eb750c

Move _merge_* calls back into vstack [skip ci]

3da60b2

Merge all attributes within empty_like

f216f1f

Look for empty_like attribute as test for vstack

bfab7dc

Copy QTable testing additions from mhvk

4c11028

Remove unnecessary call to _merge_col_meta for hstack

6909690

taldcroft added 5 commits May 8, 2017 13:42

Remove _merge_col_meta function

151965c

Make empty_like an info method

ad51f61

Check that mixin class is supported for selected operation

f0f3bf4

Use NotImplementedError for lack of masking support

a3f9e28

Add or update documentation

0399c2d

taldcroft force-pushed the table-mixin-ops branch from 7369347 to 0399c2d Compare May 8, 2017 17:42

taldcroft added 2 commits May 8, 2017 15:07

Move merge_ndarray_like_cols to BaseColumnInfo.merge_cols_attributes

7e3c62d

Expose Info classes in API docs

14a6606

taldcroft added the Ready-for-final-review label May 8, 2017

Fix sphinx error in docstring

dfab61f

mhvk reviewed May 9, 2017

View reviewed changes

Address misc review comments

672d85d

taldcroft commented May 9, 2017

View reviewed changes

mhvk merged commit 2c5964a into astropy:master May 9, 2017

bsipocz mentioned this pull request May 9, 2017

vstack, hstack, and join on QTable instances holding Quantity #5811

Closed

taldcroft added a commit to taldcroft/astropy that referenced this pull request May 9, 2017

Update changelog for astropy#5841

66980f5

bsipocz added a commit that referenced this pull request May 9, 2017

Merge pull request #6024 from taldcroft/table-ops-changelog

9a293a4

Update changelog for #5841

mhvk mentioned this pull request May 9, 2017

Support vstack of Quantity columns #4599

Closed

taldcroft added a commit to taldcroft/astropy that referenced this pull request May 13, 2017

Update changelog for astropy#5841

088d07d

This was referenced May 31, 2017

Ensure QTable is tested everywhere non-masked Table is #5821

Closed

QTable should pass all Table tests that do not involve masking #5785

Closed

astrofrog mentioned this pull request Jul 7, 2017

Semi-random test failure of QTable operations #6291

Closed

taldcroft deleted the table-mixin-ops branch October 1, 2019 20:43

		__all__ = ['join', 'hstack', 'vstack', 'unique']


		def _merge_col_meta(out, tables, col_name_map, idx_left=0, idx_right=1,

Conversation

taldcroft commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

taldcroft commented Feb 26, 2017

Uh oh!

taldcroft commented Feb 26, 2017

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!

taldcroft commented Feb 26, 2017

Uh oh!

mhvk commented Feb 26, 2017

Uh oh!

taldcroft commented Feb 26, 2017

Uh oh!

taldcroft commented May 8, 2017

Uh oh!

mhvk 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!

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!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Feb 24, 2017 •

edited

Loading