Add DataInfo get_sortable_arrays to allow mixins as key columns in join #9340

taldcroft · 2019-10-08T14:57:47Z

Should be used as the base for #9287.

mhvk · 2019-10-08T15:22:44Z

I like the idea, but first a general comment: could we not more simply rely on the presence of argsort? Time.argsort already exists and does, I think, what you want (if not, it is a bug!). One could then perhaps an info.argsort as a possible fall-back for columns that do not implement argsort directly, but I think I would stick with that name.

taldcroft · 2019-10-08T15:33:50Z

The problem is if you have (e.g. as in the test) both Time and other key columns, it becomes inconvenient to lexically sort all the columns. Just being able to argsort the Time column in isolation is not enough. The basis of the join operation is making a pure ndarray structured array corresponding to the key columns that sorts properly. Of course there are ways to do this basic operation differently, but I am not sure any of them are simpler.

mhvk · 2019-10-08T16:18:56Z

Ah, yes, I see how it helps in that sense. I'll try to have a more detailed look later.

taldcroft · 2019-10-08T19:06:46Z

astropy/table/operations.py

        raise tme


+def _get_join_sort_idxs(out_descrs, keys, len_left, len_right, left, right):


Looking again I think that out_descrs isn't needed, and len_left and len_right could be dropped.

taldcroft · 2019-10-08T19:07:16Z

astropy/table/operations.py

+    sort_right = {}  # sortable ndarray from right table
+
+    for key in keys:
+        # TODO: shape here seems out of place. Can a key column have a non-trivial shape?


This shape is not actually used now.

I wondered about this - it seems it should be impossible to sort on anything but 1-dimensional columns.

bsipocz

It all looks and very clever to me. Take my approval with a pinch of salt, as I'm not sure how serious something need to be in here so I would spot it.

bsipocz · 2019-10-10T22:32:11Z

astropy/table/operations.py

+    # a new structured array that represents the lexical ordering of those
+    # key columns. This structured array is then argsort'ed. The trick here
+    # is that some columns (e.g. Time) may need to be expanded into multiple
+    # columns for ordering here.


Thank you for these detailed comments, it really helps a lot now and will in the future, too

bsipocz · 2019-10-10T23:55:32Z

astropy/time/core.py

+        Return a list of arrays which can be lexically sorted to represent
+        the order of the parent column.
+
+        :return: list of ndarray


this is super nit picky, and don't need to be changed right now, but longer term it would be nice to use numpydoc format. If you still use pycharm, the docstring format can be changed in preferences-->tools-->python integrated tools

Agreed, done. Thanks for the hint in setting this preference. I had this set before but I think I accidentally blew away my .idea directory at some point with git clean -fxd, so the docstring format got lost.

got bitten by that, too, now I only have git clean -dfx -e .idea -e notes in my history and stick to use it from there rather than typing it in :)

mhvk

This looks very nice! Only small comments - the main one is your own one that the new sorting function's signature can be simplified.

It does seem to me that columns with non-trivial shape cannot be sorted without further information - in which case your new get_sortable_columns is just the right place to put that information. So, it would seem to make sense to insist on that. (From quick experiments, it seems argsort just takes only the first element)

mhvk · 2019-10-11T00:06:02Z

astropy/table/column.py

+
+        For Column this is just the column itself.
+
+        :return: list of Column


Like @bsipocz noted, should be something like

Returns ------- arrays : list of Column

Though I think it is fine to just delete it too, since the title already states what it is.

mhvk · 2019-10-11T00:07:47Z

astropy/table/operations.py

+    sort_right = {}  # sortable ndarray from right table
+
+    for key in keys:
+        # TODO: shape here seems out of place. Can a key column have a non-trivial shape?


I wondered about this - it seems it should be impossible to sort on anything but 1-dimensional columns.

mhvk · 2019-10-11T00:09:22Z

astropy/table/operations.py

+
+        if len(left_sort_cols) != len(right_sort_cols):
+            # Should never happen because cols are screened beforehand for compatibility
+            raise ValueError('unexpected mismatch in sort cols lengths')


Maybe use RuntimeError for things that should never happen (still good to check).

Alternatively, I think it makes sense to use assert ..., "<reason>" here, so that it does not get executed with python -O.

I went with RuntimeError. I don't actually understand why you would want to not execute this test via python -O. I must just be missing something.

I don't think it matter much, since the test is far cheaper than the rest, but I think assert is reasonable since you write right about that this "Should never happen", i.e., you have already tested in earlier code and thus here you simply 'assert' that the earlier checks were done. (I like this use of assert - it asserts assumptions made in the code that should hold, but that, given that bugs do happen, may be wrong any way - I think when you optimize for speed, it is fine to go with the assumptions...)

astropy/table/operations.py

mhvk · 2019-10-11T00:18:10Z

astropy/table/operations.py

        raise tme


+def _get_join_sort_idxs(out_descrs, keys, len_left, len_right, left, right):


astropy/utils/data_info.py

taldcroft

I think I have addressed all comments.

mhvk

All looks OK now, nice! merging....

This commit removes documentation that has been made obsolete by pull requests astropy#9340 and astropy#11127.

taldcroft added table Enhancement labels Oct 8, 2019

taldcroft added this to the v4.0 milestone Oct 8, 2019

taldcroft requested a review from bsipocz October 8, 2019 14:58

astropy-bot bot added time units utils labels Oct 8, 2019

taldcroft requested a review from mhvk October 8, 2019 14:58

Add DataInfo get_sortable_arrays and use in join

8e44e4a

taldcroft force-pushed the table-sortable branch from 66f5ed4 to 8e44e4a Compare October 8, 2019 16:51

taldcroft commented Oct 8, 2019

View reviewed changes

bsipocz approved these changes Oct 10, 2019

View reviewed changes

bsipocz reviewed Oct 10, 2019

View reviewed changes

mhvk reviewed Oct 11, 2019

View reviewed changes

Address review comments

7f7e62d

taldcroft commented Oct 11, 2019

View reviewed changes

taldcroft requested a review from mhvk October 11, 2019 11:58

mhvk approved these changes Oct 11, 2019

View reviewed changes

mhvk merged commit 04baa3a into astropy:master Oct 11, 2019

mhvk mentioned this pull request Oct 11, 2019

Use raise ... from to get tracebacks #9354

Merged

bsipocz mentioned this pull request Oct 11, 2019

Adding test for TimeSeries join #9287

Merged

taldcroft deleted the table-sortable branch December 10, 2019 23:30

eerovaher added a commit to eerovaher/astropy that referenced this pull request Aug 9, 2021

Update documentation about mixin table columns

55c2efe

This commit removes documentation that has been made obsolete by pull requests astropy#9340 and astropy#11127.

		raise tme


		def _get_join_sort_idxs(out_descrs, keys, len_left, len_right, left, right):


		For Column this is just the column itself.

		:return: list of Column

Uh oh!

Add DataInfo get_sortable_arrays to allow mixins as key columns in join #9340

Add DataInfo get_sortable_arrays to allow mixins as key columns in join #9340

Uh oh!

Conversation

taldcroft commented Oct 8, 2019

Uh oh!

mhvk commented Oct 8, 2019

Uh oh!

taldcroft commented Oct 8, 2019

Uh oh!

mhvk commented Oct 8, 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!

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants