[ENH] Refactoring and Generalizing DelegatedForecaster

[miraep8]

Reference Issues/PRs

While implementing MultiplexTransformer for #2459 to mimic the changes in #2458 it became clear that writing a more general delegator function would be helpful.

This PR technically depends on #2540.... was making sure there weren't any clashes with #2458... but if needed I can rebase and get rid of that dependency

What does this implement/fix? Explain your changes.

This replaces the DelegatedForecaster mixin with a decorator. The decorator will defer the function being wrapped to a delegated estimator if one is provided, and will also check that the current function is supported for the wrapped estimator (which should allow the decorating ability to be more general). So far it has only been implemented for Forecasters (with particular focus on making sure it works for the solution in MultiplexForecaster, but I believe it should be easy to extend to transformers as well! (Note - it now becomes very important that whatever we are wrapping the estimator in has all the same supported functions as the wrapped estimator itself, thought I think this was the case/practice before!)

this PR implements the following changes:

• removed current DelegatedForecaster and associated references to it.
• added a delegate_name attribute to BaseObject as well as get_delegate function (borrowed from DelegateForecaster)
• wrote a decorator (delegate_if_needed) which will now handle the delegation and type checking. Currently lives in sktime.utils.check_estimators - but there might be a better place for it! (Let me know what you think!)
• added a new public function to MultiplexForecaster to facilitate changing the selected_forecaster to ensure forecaster_ is also updated (Used to be done within fit(), but now you have to ensure the new forecaster_ is set before calling fit() on it.)
• Added an inheritance to BaseForecaster to MultiplexForecaster.
• Checked that the change didn't break anything (at least not locally)

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

• Overall - concerns on using the decorator vs the mixin approach???
• Suggestions on how names/where code lives could be changed to make this clearer

Any other comments?

Still have some Todos before I would consider this "ready to merge":

• extend to delegating transformers as well
• test that the new delegator approach throws the appropriate TypeError if you try to use a method not supported by the wrapped estimator.
• adding a more detailed explanation about what is going on in estimator_checks (and also where I use the decorator)

(Also note - using the decorator actually solves some of the issues that were brough up in #2458 in that the MultiplexForecaster no longer needs to dynamically set the tags in order for the fit to work since it is technically 'bypassed' in the fit process. I left the clone_tags so that get_tags on a MultiplexForecaster will return the correct tags for the wrapped forecaster (since get_tags is not delegated)- but perhaps this could be delegated as well, and we wouldn't need to set the tags dynamically at all. Thoughts??)

Also - very much ok if we don't refactor in this way! I stopped before applying it to the transformers as well to get feedback on what you all thought of this solution! :)

[miraep8]

Thank you for the feedback! I am working on implementing these/other changes this weekend/will request feedback when they are integrated/the code is passing the CI. Just to follow up on some of your comments in the meantime:

• Thanks for the suggestion - I will check out the make_mock_estimator, would be great to be consistent with how decorators have already been used in the code base.
• I agree/will move the delegation functionality out of BaseObject and into BaseEstimator (since it will be useful for both Transformers and Forecasters).

indeed the decorator will produce unexpected results for all methods and attributes that return self, including fit, update, set_params, is_fitted. This could be solved by an argument to the decorator, e.g., returns_self?

• Good suggestion, will try it out!

I would keep the _y private, I think this should not be exposed to the users. Also, this seems like a change that is orthogonal to the other content in this PR?

• Ah, allow me to explain:
  • to me making the y property function ends up being useful as it allows me to delegate fetching the ._y property to the wrapped estimator. There might be another way to do it, but this was a very simple solution.
  • I chose to do it this way because it seems like similar property functions already existed for ._cutoff/.cutoff and fh/._fh, so I felt the change was in keeping with the existing code/I was unsure of why ._y should be special.
  • this change actually allows the test functions (which motivated me to make this change in the first place as they were breaking on the code without the ability to delegate getting ._y) to actually reference the .y property rather than the ._y property (which they were doing before). However I can change it back if you prefer.

[aiwalter]

I think __init__ is not the right place for attributes. See also here: https://scikit-learn.org/stable/developers/develop.html#instantiation and here https://scikit-learn.org/stable/developers/develop.html#estimated-attributes

[miraep8]

Thanks @aiwalter, I definitely want to be consistent! Think I have a working work-around now so that defining self.best_forecaster_ no longer happens in init.

[aiwalter]

why do you rename here to fit()? This means you overwrite the parent fit() and I think we should not do so 🤔

[miraep8]

So, it was actually intentional that I overwrote the parent fit(), though I no longer do so now. :) Long story short - the overwriting was a way to get around the delegation of fit(), but turns out this solution requires some sort of hacky workaround anyway.... so now this is _fit() again, and we no longer attempt to delegate BaseGricCV's fit() - though we do still delegate the other functions! If you get a chance I would appreciate your thoughts on the new version! :) Thank you for flagging this, it was kind of a subtle point, and I think the new version that doesn't do this is cleaner.

[fkiraly]

I agree/will move the delegation functionality out of BaseObject and into BaseEstimator

Can you explain why you put the "delegate" functions there in the first place? I thought these should apply only to the inner methods such as _fit etc.

Ah, allow me to explain

Hm, I think it might be a good idea overall, but it is a separate change and changes the base interface.
Also, a lot of things reference _y, including possibly in descendants which you do not catch with a search/replace. It is also a documented interface point which developers have relied on for long, so it cannot change without a major communication element.

So, could you open a separate PR where you apply this change to _X and _y and explain the consequences and rationale in the PR header?
You can use that PR as a basis for this one, but it might be easier to get this one in without a change in the core interface.

Formally, it´s a core interface change (how the base class functions), so it requires a STEP document (whicih can be in the PR for small changes).

[miraep8]

I agree/will move the delegation functionality out of BaseObject and into BaseEstimator

Can you explain why you put the "delegate" functions there in the first place? I thought these should apply only to the inner methods such as _fit etc.

They actually apply to the non - underscore methods directly. As I mentioned in my initial comment, the way this is implemented actually allows the user to bypass the call to fit on some of the wrapped estimator types, which was intentional, and seems to work well! As to why I put it there in the first place - it was a simple mistake/I totally agreed with your point to move it - hence why I was happy to move it when you pointed it out! :)

Hm, I think it might be a good idea overall, but it is a separate change and changes the base interface.
Also, a lot of things reference _y, including possibly in descendants which you do not catch with a search/replace. It is also a documented interface point which developers have relied on for long, so it cannot change without a major communication element.

So, it is worth noting this change doesn't actually break anything for anything referencing ._y.... As I said in my mind it was essentially useful for providing a workaround for the tests/again as I said seemed in line with what was in the code already. :) However, it doesn't need to be this way and we can do one of the following instead:

• exclude the wrapped estimators from the y tests,
• ensure these attributes ie (is_fitted, y, etc get copied over and applied to the wrapping delegator).
• write the workaround or some version of it that lives in HeterogenousMetaEstimator rather than in BaseEstimator which would allow it only to apply only to these sort of estimators.

In my mind - the best of these options is number 2, so that's the one I will plan to go with. It should also help with the ._is_fitted bug I was getting if applied correctly. Let me know if you feel differently. I think if we go with number two it also makes more sense to stay in this PR.

[fkiraly]

So, it is worth noting this change doesn't actually break anything for anything referencing ._y.... As I said in my mind it was essentially useful for providing a workaround for the tests/again as I said seemed in line with what was in the code already. :)

Oh, it would be an additional interface point?
If yes, what would be interfacing it? I´m worried about splitting interface to the same thing across two points.

Let me know if you feel differently. I think if we go with number two it also makes more sense to stay in this PR.

I actually do feel differently, since we are probably diverging in conceptualization of what the delegator should do.

In my mind, it's a mixin - you would not use it directly, but inherit from it, in order to patch methods through quickly. Then, you would override methods which you would not want to have a patch through logic. As every mixin, it does not work by itself, but only in combination with some custom implementation.

Besides the patching through of methods, I don't think anything else should happen, especially not things that are hard to override such as copying _y or _cutoff back and forth. Imo that can have unintended side effects which we want to avoid, or at least control in a concrete implementation (instead of inviting it by default).

[miraep8]

Hey!

So I have made a few changes - though it is important to note it is only passing tests now because I commented out the make_mock_estimator test! I wanted to show that the remaining issues were localized to that test only (and the issue here is caused by the fact my delegation decorator interferes with the decoreator in MockEstimator's method_logger... which I need to look into a bit more to figure out!). I have the full intention of fixing this issue, but wanted to demonstrate that the other issues (which were more central to how the delegation function actually works!) are now resolved!

Oh, it would be an additional interface point?
If yes, what would be interfacing it? I´m worried about splitting interface to the same thing across two points.

Well yes! Though as I mentioned I ended up changing the way I did this..... It is worth noting though, this is how the code already works for _cutoff, _fh and _is_fitted, so if you have an issue with me doing it this way it could be worth checking out the code which already has 2 references points this way. ie:

    @property
    def cutoff(self):
        """Cut-off = "present time" state of forecaster.

        Returns
        -------
        cutoff : pandas compatible index element
        """
        return self._cutoff

I also implemented your suggestion to have a return_self parameter to the decorator. This works well in my opinion/was a good suggestion. Let me know what you think!

As for how to set the attributes - I have work around via an additional attribute - called _attr_to_copy which allows control over which of these attributes are copied over from the wrapped estimator to the main estimator. I would appreciate your thoughts on this solution. In my mind these are some of the advantages/reasons for doing it this way:

• by delegating fit instead of _fit we can actually get around the need to dynamically set tags. To me this seems like a great solution for all of the composite estimators (we can also delegate certain tags to match the wrapped estimator! I have not done this yet, but I can if you like it!).
• but by delegating fit however - we never run the input checks on the main estimator (which again is an advantage because of the above). Thus certain attributes ie y, is_fitted etc don't get properly set. So if we want to access them from the main estimator, we then need to copy them over from the wrapped estimator if the input is accepted, this is a way to do so!
• by having the names of which attributes should be copied in an attribute itself - I hope this to some degree addresses your desire to have "control in a concrete implementation (instead of inviting it by default)." but let me know!

Again as always - no rush to check out these changes/I really appreciate all the helpful advice so far! Also - if we really think this approach doesn't work well, I am happy to scrap this PR. Though as it stands now it seems like a reasonable approach/solves some problems the mixin approach might not! :) (though I am obviously biased).