I would expect this to be shared among implementations. I think that the history output of the fit function should have all of the information here. Are there choices that are likely to change between implementations?

Yes, I think so. Initially, the structure of my IncrementalRandomizedSearchCV differed from Hyperband, as my results were simply results = fit(...), and not results = {model_id: fit(...) for ...}.

Now that both have the consistent results type of {model_id: (info, models, history)}, more of this should be sharable.

same applies to #356 (comment)

In my implementation of _get_cv_results, the bracket key (model_id here I think) was only required to make it so the model_ids were unique across brackets. This was important for downstream processing; I wanted unique model IDs for each bracket, not shared (and isn't the cleanest code).

This solution can be used with the "consistent results type of {model_id: (info, models, history)}". The result I came up at _hyperband.py#L315-L318 is something like

for loop_id, (info, models, hist) in results.items():   # results defined in comment above
    for h in hist:
        h["model_id"] = "loop{}-{}".format(loop_id, h["model_id"])

I would expect the additional_calls function to be a method of this class that developers would override.

I think that we probably don't need users to define model_ids. The fit function will do this for us.

I do think that we need a way to decide initial parameter lists. This might be an overrideable method.

@mrocklin does _get_ids_and_args make sense now? Initially the building of param_lists and additional_calls was two separate methods, but it seems like they may be dependent on eachother. So I figured having a single method to override the things passed into fit made the most sense.

This is the only mandatory method that has to be implemented by subclasses now.

I think that this should be easy to make a default for. I would pass over all of the models in the resulting models dict and then pick the one with the best score coming out of info

I would also expect this to be somewhat standardized among different implementations. What does this need to do?

I'm not really sure. For Hyperband, it's something like

        self.history_ = sum([SHA._history for SHA in SHAs.values()], [])
        self.metadata_ = {
            "models": sum(m["models"] for m in meta),
            "partial_fit_calls": sum(m["partial_fit_calls"] for m in meta),
            "brackets": meta,
        }

This could probably have a default implementation that extracts the results that will likely be useful to all base classes (I'm just not sure what those items are at the moment).

I think that my objective here is for users not to have to write too much to make things work. Some of these hooks should probably be optional.

In my implementation of _get_cv_results, the bracket key (model_id here I think) was only required to make it so the model_ids were unique across brackets. This was important for downstream processing; I wanted unique model IDs for each bracket, not shared (and isn't the cleanest code).

This solution can be used with the "consistent results type of {model_id: (info, models, history)}". The result I came up at _hyperband.py#L315-L318 is something like

for loop_id, (info, models, hist) in results.items():   # results defined in comment above
    for h in hist:
        h["model_id"] = "loop{}-{}".format(loop_id, h["model_id"])

There are two parts to this class: the method of choosing parameters, and the incremental/adaptive method implemented (currently remove_worst). I'd rather the name reflect the incremental/adaptive component: maybe something like KillWorstModelCV?

I think that this is only up here as a strawman to see what things look like when extending the class. I don't think that the remove_worst function is a particularly pragmatic approach. It was just something simple that was small enough for me to put into a docstring.

I would design things so that the IncrementalRandomizedSearchCV class has remove_worst as a method, rather than as a return value of _get_ids_an_args. I think that the remove_worst method is likely to be the main point of variance between different classes, and so is something that people will probably expect to specify at the class level.

(also, just to be clear, we shouldn't actually publish a class with the remove_worst function as a genuine solution. It's not a good solution)

I also don't understand why this returns a dictionary.

The Hyperband implementation is weird in that they'll want to call fit several times rather than just once (which I think will be more common). Additionally, Hyperband will want to call fit many times concurrently, so we'll need to use coroutines for this.

Short term I suggest that we implement classes for fitting one model, RandomSearch, and SuccessiveHalving using the functions found in this notebook. Then we look at Hyperband as a special case that has to call fit a few times and see what extra methods might have to be pulled out.

I'm happy to take a look at this later today as well if that would be useful.

In short, I think that this approach is more complicated than necessary specifically because it is targetting Hyperband first and because Hyperband is a bit of a special case. I think that we should target some of the simpler situations first, build conventions around them, and then see how we can add release valves for Hyperband to also work.

My reply seems to have been lost.

tldr: that seems sensible. I agree that Hyperband's multiple calls to fit complicated this PR, and I don't know how representative that will be of other strategies (I assume not).

It looks like the methods above are untested.

I can put some together quick.