Table: Add methods 'join' and 'with_column'

[janezd]

Issue

Resolves #5115.

Please comment the method names. 'join' reminds of SQL's join, but it's kind of correct.

Includes

• Code changes
• Tests
• Documentation

[codecov]

Codecov Report

Merging #5251 (5429e1c) into master (0568368) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5251      +/-   ##
==========================================
+ Coverage   85.18%   85.22%   +0.03%     
==========================================
  Files         300      300              
  Lines       60979    61037      +58     
==========================================
+ Hits        51946    52016      +70     
+ Misses       9033     9021      -12     

[ajdapretnar]

Thanks for this one! 🎉

Few comments:

1. I would prefer to name the method add_column. with_column really confused me.
2. I would add docstrings. 🤷 (I can do it, if you want.)
3. Why does this fail?

iris = Table(iris)
iris1 = iris[:,:2]
iris2 = iris[:,2:]
Table.join(iris1, iris2)

[janezd]

1. I would prefer to name the method add_column. with_column really confused me.

Inspired by Kotlin's convention (e.g. withIndex). I'm not too happy with this name, but I didn't like _add_column and similar, because it suggests that the method adds a column to this table. I'll be happy to rename, but the name mustn't mislead.

2. I would add docstrings. 🤷 (I can do it, if you want.)

Was planned. Now done.

3. Why does this fail?

iris = Table(iris)
iris1 = iris[:,:2]
iris2 = iris[:,2:]
Table.join(iris1, iris2)

Wow. I was writing the tests and the code in parallel, and solved some problem by using an empty array of appropriate dimensions, but I accidentally just used the same dimension as the array in tests, soempty((5, 0)) was hardcoded. :)))

[lanzagar]

so empty((5, 0)) was hardcoded. :)))

Everybody knows that hardcoded things should be fitted to iris. Then nobody complains for a long time, because when they test the code it always works ;)

I also don't particularly like the name with_column. Maybe the effort to be so precise is a bit unneeded here, since unlike the differences between sort/sorted and similar we are talking about a method of the Table class which AFAIK we decided should be immutable in all such cases. Maybe that sacrifice at least allows us to use the most obvious name and not be misleading?

On the other hand join is a bit misleading for me, because I would definitely expect the option to join based on a unique identifier and not just sequentially. In connection to this: the method above (concatenate) used to allow axis=1 which we removed - do we remember the reason? And should we re-enable that now by calling join when axis=1? Or to avoid the name join just not have a separate method at all and use concatenate(..., axis=1) like in the olden days?
Sidenote: if we do keep two methods, I do not like the inconsistent signatures *tables vs tables

And finally: YAY, we are getting a method to add columns :)

[janezd]

add column

OK, I'll try one last appeal: doesn't this look nice?

amended = iris.with_column(label_var, label_col)

As opposed to

amended = iris.add_column(label_var, label_col)

In the second case, I'd expect amended to be None. (But it may be just me. Yesterday I thought for a moment that ndarray.flatten works in place!)

If I haven't convinced you, say so and I'm renaming it to add_column.

join

I wasn't sure about join, either. I still decided to have it as analogous to str.join.

But I agree on merging this with concatenate with axis=1 (and obviously using tables rather than *tables)

I forgot the reason for it's removal. I believe it was a mess. We probably thought it would be cleaner and didn't realize how much we'd miss it.

[ajdapretnar]

I see your point, but I am somehow not used to methods describing the results, but rather the operation. For example, table.to_dense() returns a dense table. If it described the result, it would be table.as_dense(). concatenate() and join() both describe the action. If they described the result, they would be concatenated() and joined(). Or am I mistaken? Perhaps @markotoplak and @VesnaT can add their two cents.

As for join(), I was equally surprised it didn't accept the key. pandas' join() joins by key if given, else it joins by index. Is this the right moment to reiterate how pandas will solve everything?

[janezd]

Decision: concatenate with axis; add_column.

[lanzagar]

We should probably think about using the new functions (especially add_column) in the many places where it is done manually now.
We obviously don't need to wait and can merge this beforehand, after the minor changes mentioned (docstring and name).

[lanzagar]

The name of the new table should also be set, otherwise it is 'untitled'.
The same approach can be used as for axis=0:

names = [table.name for table in tables if table.name != "untitled"]
if names:
    table.name = names[0]

[janezd]

I extracted the common code from _concatenate_vertical and _concatenate_horizontal to concatenate.

All changes are in the second commit.