IndexSelector: use dynamic options

[achimgaedke]

addresses loading timeouts for large datasets as described in #205

This is rough and ready, keen to get some feedback.

[oegedijk]

Cool, really nice approach! I actually noticed that dynamic dropdown thing in the documentation the other day as well. Very nifty!

Wonder if there are any performance tradeoffs though such that for small datasets (or rather maybe memory usage of self.idxs) we would use the old_fashioned dropdown and for larger ones the dynamic one.

But if it is perfomative enough than we can just default to the dynamic one...

[oegedijk]

This seems quite a bit slower, but has the benefit of being case sensitive:

[oegedijk]

Probably makes sense to wrap this into an IndexDropdownComponent so that is easy to replicate across components.

[oegedijk]

so this seems to work (check the feature input component index selector on the whatif tab)...

[oegedijk]

hmm, tests seem to fail on ubuntu. May check tomorrow...

[achimgaedke]

Glad you like it.

Why not use the IndexSelector everywhere?

Maybe there's something I'm missing in the code. It's likely that I actually haven't found all index selection locations.

The limit of options returned (1000) is probably best to make configurable... a bit like plot_sample.

[achimgaedke]

Btw, very pleased with the load time of the application utilising 1M data points.

[oegedijk]

Good question, and answer is that I forgot I had already written IndexSelector, but am apparently good at reinventing the wheel :)

[oegedijk]

I was looking at the FeatureInputComponent which didn't use the IndexSelector, my bad. Anyway, will have a look later. Also at this weird stochastic ubuntu bug.

[oegedijk]

Okay, I now extended the IndexSelector to all existing ExplainerComponents, and also added a functionality that that when the index is set directly the options simply get set to [index]. Before if the index was set from e.g. a random index selector and the chosen index was not already in the dropdown, it would not show.

Since you have ready access to a million row index dataset, could you test performance of this self.explainer.get_index_list().str.contains(search_value, case=False)][:self.max_idxs_in_dropdown].tolist() vs the islice approach you had first?

I would like search to be case-insensitive, so if islice is much faster on big datasets, then we could add an .index_list_lower() and compare search_value.lower() to it...

[oegedijk]

bumped sklearn to 1.1 and python to 3.8 and voila tests are passing :)

[achimgaedke]

Thanks for keeping up the momentum on tbis and #217. I'm keen on trying it out, but realistically i might have a chance to get to it mid next week. The next bottle necks for 100s of thousands of data points are the ROC and PR AUC plots, esp the latter. I haven't had a closer look for the reasons. Not having the index repeatedly/at all in the JSON layout file is a great step forward. Cheers!

…

On Mon, 23 May 2022, 20:46 Oege Dijk, ***@***.***> wrote: bumped sklearn to 1.1 and python to 3.8 and voila tests are passing :) — Reply to this email directly, view it on GitHub <#219 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTGKNRT3VFSBKULN77XUIDVLPHBVANCNFSM5VVCII6Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[achimgaedke]

I have reviewed the changes.

I propose some changes to the IndexSelector class from a software-engineering perspective, mainly from a consistency perspective and a bug in the conditions between the layout and the callback creation.

There's one difference in the use of max_idxs_in_dropdown: The original fix proposes to send dropdown options always as a callback result when requested. This version goes the middle way: allowing the dropdown values to be included in the app layout (multiple times).

Is a purely callback-based option too slow in some cases? If not, I'd suggest keeping only that one.

All in all, I'm worried about startup performance (and hitting a timeout with JSON becoming too slow to load) rather than runtime performance. Do we want two different thresholds?

I am not too concerned with the CPU performance difference of iclice vs pandas.DataFrame.contains for now. One would also need to consider memory consumption.

[oegedijk]

I think some subtle bug may have been introduced with the cleanup, but will have a closer look tomorrow..

thanks again for the work!

[oegedijk]

Alright, looks good to me, let's merge it!

Thanks again for this awesome initiave!

Will give you a shout out on linkedin once I'll release this version...

[oegedijk]

just released it as version 0.4.0