Skip to content

Support vstack and similar operations for SkyCoord #6348

Closed

Description

@eteq

I use SkyCoord as a mixin quite a bit in tables - it's very common to have catalogs with RA/Dec coordinates as one of the axes, and of course it's convenient to just drop SkyCoord's directly into that.

However, if I try to vstack two tables with SkyCoords as one of the columns, I get the dreaded:

NotImplementedError: vstack unavailable for mixin column type(s): SkyCoord

This seems like it should be straightforward given that SkyCoords pretty much behave like table columns... Of course I'm sure it's not that simple... but it's a pretty important use case, and maybe it's time has come?

cc @taldcroft @mhvk

(this might be a duplicate of a more generic "make mixins of all types work"... but I didn't find an obvious one)

Activity

  1. added this to the v3.0.0 milestone on Jul 7, 2017
  2. taldcroft commented on Jul 7, 2017

    @taldcroft
    Member

    @eteq - completely agree. The generic way of supporting mixin operations like join, hstack and vstack requires that the object be mutable because the code first creates an empty instance of the object with the right shape and attributes, then fills in the elements appropriately. See e.g. the code here for vstack. It makes a new column based on the list of columns to be stacked using the Info.new_like() method. This is asking the mixin class to do all the meta merging and figure out if the inputs can really be stacked and what the output should look like. Then it sets the output via slice or fancy indexing array assignment operations.

    We need to set items in SkyCoord in this context. So this is the time to discuss making SkyCoord mutable, which I know you have a strong opinion about. Note that in theory the vstack code could do this slice setting with a context manager that temporarily enables mutability.

  3. eteq commented on Jul 25, 2017

    @eteq
    MemberAuthor

    first creates an empty instance of the object with the right shape and attributes, then fills in the elements appropriately.

    Oh, I didn't realize that @taldcroft ... that explains why this is harder. I guess I don't understand why this is a necessity, though? That is, why can't it instead (optionally) create the correct thing initially and drop it in? That would be a lot easier to implement as it stands...

    Note that in theory the vstack code could do this slice setting with a context manager that temporarily enables mutability.

    Hmm, perhaps I could get behind that? Perhaps a topic for elsewhere, though, depending on the result of the above.

  4. taldcroft commented on Jul 26, 2017

    @taldcroft
    Member

    That is, why can't it instead (optionally) create the correct thing initially and drop it in?

    I assume that you mean something like the new_like() method would create the new stacked SkyCoord column from the input columns by first doing the vstacking on the individual data attributes (e.g. ra, dec, distance) and then instantiating with the vstack'ed inputs? If so...

    The problem is that this model does not generalize well to the case of missing values. In other words, there is no guarantee when you stack two tables that all the columns match, so the new column may be longer than the sum of the length of the inputs. Likewise for hstack you don't know that they are all the same length, and for join you don't know that there won't be missing rows in the join.

    So this makes things ugly. There is currently the table high level operation code (for vstack, hstack, join) which knows all the details for assembling the output for any case. You would then need to move that functionality to be specific to each class that can be an input, effectively having vstack, hstack, and join functionality for every supported table mixin class. This is certainly possible but I would argue it is not preferred.

    Instead if you stipulate that an object has a setitem that supports slice and fancy index setting, then all the specifics of vstack, hstack, and join stay in the table code where they belong. The only thing specific to the class is new_like, where they class knows how to construct an empty object of the right length given the input columns. But it doesn't have to know what is going in to it.

  5. bsipocz commented on Nov 30, 2017

    @bsipocz
    Member

    @eteq @taldcroft - Would the use case of matching two tables fall under this issue, too? I mean both have a SkyCoord column, but with some non zero matching separation. Would joining the two of those tables be possible when this gets implemented, or will require a convenience function (with probably a seapration argument)?

  6. taldcroft commented on Nov 30, 2017

    @taldcroft
    Member

    Would the use case of matching two tables fall under this issue, too? I mean both have a SkyCoord column

    @bsipocz - yes, sort of. You would need to do pre-processing of the table to do the matching in advance and have something like an integer id column that then allows join-style matching. A fundamental part of the table join algorithm is being able to sort on the match key, so a SkyCoord won't work.

    However, if you did make the id columns and then used those as the key column, then this functionality (along with what I have argued is a pre-requisite, #6394) would do the trick.

  7. modified the milestones: v3.0.0, v2.0.4 on Dec 22, 2017
  8. mhvk commented on Dec 22, 2017

    @mhvk
    Contributor

    Removing milestone, since we're not going to make this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions