FEA Joiner add many-to-many joins

[jovan-stojanovic]

Fix #629 and fix #628

[LilianBoulard]

Thanks for the nice implem!
I've added a few minor comments, otherwise it's all good :)

[GaelVaroquaux]

A few comments.

Do we have an exemple for the new functionality? I think that we would need a dedicated example, for instance with GPS coordinates.

It should mention the following words (for search engine discovery): "spatial join", and also "join keys across multiple columns"

[GaelVaroquaux]

Thanks. This is exciting!!

A bunch of comments, but mostly cosmetics

[GaelVaroquaux]

I wanted to suggest to do a GridSearch here to set the match_score hyper parameter of the Joiner. But unfortunately this example is 11mn long, so we can't really make it much longer.

Do you know what takes time? Maybe you can run a profiler on it. It would be very beneficial to decrease run time in general

[jovan-stojanovic]

Yes, it's mostly the GapEncoder. Especially when working on the ID columns, which is why I dropped them (#585).
I hope this will get much better after speedups #680

[GaelVaroquaux]

Maybe a small example of something that looks like a spatial join here, so that people find out that it is possible.

[GaelVaroquaux]

Can we also:

• document in fuzzy_join that it works with multiple keys (this is non trivial)
• add a tiny example in the docstring that demos this
• add a test that asserts that it works

[GaelVaroquaux]

The doc building timed out :(

[GaelVaroquaux]

It seems that the docs are still timing out: the example is taking too long.

@jovan-stojanovic : Can you run it on your computer to find out what is making the example run so long. Maybe at verbosity or a bit of timer, so that we understand

[jovan-stojanovic]

It seems that the problem is now downloading the data from figshare (which was not this morning) :(

[GaelVaroquaux]

It seems that the problem is now downloading the data from figshare (which was not this morning) :(

OK. In a sense, this is good news, because it means that there is no fundamental problem with the example. To make examples more robust to data download difficulties we should: 1. be writing fetching functions that cache to disk 2. implement dataset caching on the doc building, as in nilearn https://github.com/nilearn/nilearn/blob/main/.github/workflows/README.md#dataset-caching Item 2 above is certainly outside of the scope of this PR. Do you think that you can do item 1?

[jovan-stojanovic]

1. be writing fetching functions that cache to disk 2. implement dataset caching on the doc building, as in nilearn https://github.com/nilearn/nilearn/blob/main/.github/workflows/README.md#dataset-caching Item 2 above is certainly outside of the scope of this PR. Do you think that you can do item 1?

Yes, definitely, will do it, I hope this helps for now. But item 2 would actually solve this problem in all our examples.

[GaelVaroquaux]

But item 2 would actually solve this problem in all our examples.

I've added an issue so that we don't forget about item 2: #693

[Vincent-Maladiere]

I agree with @Vincent-Maladiere that we need to harmonize the API. It's important.

@Vincent-Maladiere : how often do you think that we will need multiple joins, in particular with the same set of tables?

The dilemma is the following: I wouldn't be surprised that, in the long run, we can implement optimized version of multiple joins/aggregations by passing them to the underlying engine and letting it optimize the total. But on the other hand, using list of columns names for multi-key joins simplifies a huge amount doing multi-key joins, which is something that is bound to be needed some times. I don't know which way to go.

Opinions?

The multiple join feature is probably niche, now that I think about it. Most users won't need it, but every user will benefit from a simpler API :)

If we'd really need it, my intuition is that we could create another transformer later, like MultiJoinAgg.

[Vincent-Maladiere]

LGTM once the dataset downloading issue is cleared, I really like the API simplification :)

[jovan-stojanovic]

Note that we need to add the cache for example data and change the name of example 4 in follow-up PR's.