Move allow_* flags to Campaign

[AdrianSosic]

This PR builds upon #412 and finalizes the metadata migration from search spaces / recommenders to Campaign, making Campaign the only stateful class:

• All allow_* flags are now passed directly to Campaign, strengthening the role of Campaign as the class for metadata handling / tracking the progress of an experimentation path.
• A corresponding deprecation mechanism is set into place.
• The documentation gets updated.
• allow_repeated_recommendations is renamed to allow_recommending_already_recommended because i) the original name was imprecise in that it also suggested that repetitions are disallowed within a batch and ii) the new follows the same pattern as the other two flags.
• The flags are now context-aware, i.e. setting flags in contexts where they have no effect now raises an error. This avoids surprises on the user side, which have been reported multiple times.
• Replaces the private class AnnotatedSubspaceDiscrete with a new private class FilteredSubspaceDiscrete taking over the original role but without the necessity of being metadata aware.

Coming next

The changes introduced here and in #423 bring significant improvements from a user interface perspective in that:

• Metadata handling now happens entirely behind the scenes
• No more fiddling with internal low-level objects (like setting Boolean values in metadata dataframes) is necessary to control candidate generation is necessary. Instead, there is now an interface that enables control via user-level objects such as Constraint objects and candidates dataframes.
• The pandas part of the current discrete search space implementation is less entangled in the code base

This paves the way for upcoming enhancements:

• A SubspaceDiscreteProtocol is already in sight, which allows seamless integration of other backends (polars, databases, etc) and will help us complete the ongoing polars transition.
• An improved user interface for manipulating existing state spaces a la SubspaceDiscrete.filter(constraints), which can also become the backbone of the current constraints logic

[AdrianSosic]

@Scienfitz: I remember us having a longer discussion about this part in one of the previous PRs. With the new changes, I think the semantics are now considerable easier but we probably still need to iterate once again to align what the desired behavior should be

[Scienfitz]

wait now the warning is printed whenever pending experiments is provided independent of the flag, that can be annoying

eg what happens if I pass pending experiments because I want to exclude them (flag on True), I'd always get this warning now or not?

[AdrianSosic]

Your conclusion is correct but I think your expectation is wrong. That's the point I tried to convey in today's meeting but didn't succeed :D Let's go trough the cases:

Recommender Level

Here, the flags don't exist, and we need to disentangle the two concerns of

1. informing the algorithm about pending points. This is done by passing pending_experiments to recommend
2. Excluding pending points from the candidate set. This is the filter part that is still missing. As I said, controlling what is recommendable on the search space level now always goes via filtering, i.e. creating a new object, since there is no more internal state we could control. So in the future, we can flexibly control this (in general, not just for pending points!) via recommender.recommend(searchspace=searchspace.filter(...))

If you consider the above, then the conclusion is that RandomRecommender(pending_experiments=df) should always raise the error, because the algorithm can't handle the info and excluding experiments would go via the filtering route instead.

Campaign Level

Same separation of concerns:

1. Informing about pending points is done in the same way, by passing pending_experiments to recommend.
2. Excluding pending points.

The second part is probably where the misunderstanding is: Excluding the pending points by passing pending_experiments + setting the flag to False is not just excluding the pending points but also informing the algorithm, so you'd be mixing both concerns. And in the non-predictive recommender context, one of the two concerns doesn't apply / make sense. So the warning would be justified. If you really only wanted to exclude, you'd go via toggling, which would be fine. But really the scenario is a bit exotic anyway because if you have pending points, you wouldn't want to use a nonpredictive recommender in the first place. And this is exactly what the warning tells you!

Does that make sense?

[AVHopp]

If I get your points here right, I'd also favor the variant where warnings are always being printed. If people are bothered by this, people can still globally deactivate warnings.
In general, I think that we should als re-visit in general how we handle warnings as I think there are quite some places where people might want to deactivate one type of warning while keeping another one. But this should not be done here :D

[Scienfitz]

I dont think the words pending experimetns were mentioned once in the meeting, so not sure what you refer to...

Also, the argument that something is only possible if the imagined future with a filter method need to be discarded. We already filter now, just call it toggling, there is no need to defer anything into that imagined future because of it

If we look at the defacto state of the code now, non-pred recommenders can be passed pending experiments and could create this perma warning - This is not good, I remind you of the situation where the simulation module raised thousands of warnings because of the intial data. While its true that it can simply be discarded, users are confused and wonder rightfully whether they did something wrong, but in reality it was just us overly eager raising warnings.

So it seems simply to me the cleanest solution is pending experiments arg needs to be removed for non-pred recommenders (in the raising error sense to not meddle witht he protocol). The warning in recommenders becomes obsolete then. The campaign needs to decide whether it passes pending experiments or uses toggling or raises a warning if the respective combo appears - this shifting of responsibilities is akin to what you did with the other flag warnings - and again I dont see how that should be affected by further changes to renaming/moving our toggling/fitlering in an imaginary future, these imaginations relate to seachspace changes, my args are purely based on cmapaign-recommender topics.

[Scienfitz]

does it make sense to you?

• let non-predictive recommenders throw an error if provided pending expeirments
• make campaign take care that no such error is actually thrown if recommend is called via campaign. the campaign can decide its warning/error logic based on the old flags just as the logic was inside recommenders before
• the protocol change is not necessary for that

[AVHopp]

Do you need an additional opinion here? This discussion goes on for quite some time already. Or do we want to discuss this in our Meeting this week?

[AdrianSosic]

Let's align once and for all in the dev meeting 🙃

[AdrianSosic]

Christmas break is over, so back to work here 👷🏼 Have just finished the necessary preparations for the approach summarized above in #457 and implemented the corresponding logic. Please let me know what you think.

[AdrianSosic]

Note that the PR has been rebased on the still pending #457 to achieve this, hence the large diff

[AVHopp]

Very brief set of initial comments. More to come.

[AVHopp]

LGTM, just some minor questions/comments

[Scienfitz]

wait now the warning is printed whenever pending experiments is provided independent of the flag, that can be annoying

eg what happens if I pass pending experiments because I want to exclude them (flag on True), I'd always get this warning now or not?

[Scienfitz]

the fact that pending exps can still be excluded for those recommenders via the allow flags of the campaign needs to be mentioned here (which is roughly the updated equivalent of what you've deleted)

[AdrianSosic]

Waiting for the outcome of this discussion first

[AdrianSosic]

Done. Tell me how you like the new getting recommendations user guide and the faq. Probably, some content should be migrated from the campaign guide to the new one, but right now I'd prefer to merge the PR asap for the new release.

[AVHopp]

Since most if my wishes have been implemented, here you go with your approve :)

[AdrianSosic]

@Scienfitz, @AVHopp: I vote to also set the default here to True by default. Rationale:

• That way, the default behavior of the campaign is stateless (in terms of the flags), which I think is the more reasonable / less opinionated choice.
• In particular, it doesn't have any severe consequences unlike the alternative. That is, seeing the same recommendation pop up again and then making the conscious choice of disabling it (e.g. by changing the flag value on the running campaign) is absolutely fine while, on the other hand, not knowing that certain candidates are excluded from the get go is really bad.
• The above doesn't happen in the average campaign, but it has kicked me off track already many times (its exactly like the statefulness issue with the searchspace) where you simply don't expect it. For example, the last example from the new user guide wouldn't work because the first recommendation will block the subsequent ones – I bet you also wouldn't have noticed that!

   campaign = Campaign(searchspace_full, objective, measurements)
   campaign.add_measurements(measurements)
  
   # Recommendation with full search space
   campaign.recommend(batch_size)
  
   # Recommendation with reduced search space
   campaign.toggle_discrete_candidates(pd.DataFrame({"p": ["C"]}), exclude=True)
   campaign.recommend(batch_size)