[ENH] deep argument for get_fitted_params

[fkiraly]

This adds a boolean deep argument to get_fitted_params which behaves precisely as the argument of the same name, of get_params (see docstring in this PR). Mirror of sktime/sktime#4113

Besides the additional user option, this would make a subsequent translation of the get_params and get_fitted_params interfaces for the meta-estimator much simpler due to the congruence of the meta-estimator utility, see sktime/sktime#4110

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.93%. Comparing base (00e08c7) to head (ad23d05).
⚠️ Report is 406 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #117   +/-   ##
=======================================
  Coverage   81.92%   81.93%           
=======================================
  Files          30       30           
  Lines        2235     2236    +1     
=======================================
+ Hits         1831     1832    +1     
  Misses        404      404           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[RNKuhns]

Requested minor change to docstring.

Also, looking at this functionality more closely. I think we should consider how we want to use GET_FITTED_PARAMS_NESTING or whether we want to use one of two other options:

• Use tag to record this functionality (seems to fit definition, since it is developer instructions for getting things working (rather than config)
• Move to a more duck-typing approach. If a component implements get_fitted_params, we get its fitted parameters.

This wouldn't be a change in this PR, but something we should think about updating ahead of the v0.4.0 release in February.

[RNKuhns]

Maybe have this setup to use bulleted list?

How about:

def get_fitted_params(self, deep=True):
    """Get fitted parameters.

    Requires estimator to be fitted.

    Parameters
    ----------
    deep : bool, default=True
        Whether to return fitted parameters of components.

        - If True, will return fitted parameters for this estimator, plus parameters of components.
        - If False, only returns the fitted parameters for this estimator.       
    
    Returns
    -------
    fitted_params : dict of fitted parameters
        Dictionary where keys are str names of fitted parameters and values are the fitted values.
        Parameters of components are indexed as [componentname]__[paramname].
    """    

[RNKuhns]

Note that I also suggested minor updates to returns.

[fkiraly]

Re the update to returns: think it's worth pointing out the recursion explicitly.

If you go over the abridged form, it is not clear to a reader whether recursion happens or not - strictly speaking, taking the description at face value with brutal mathematical precision, recursion would not occur, unless it is additionally stated.

[fkiraly]

apologies - I realize my answer must not make sense, since I have mixed up the get_params pull request with this one, in my memory.

I have now adopted your suggestions, and also improved description coming from the get_params PR (which includes your suggestion for the return).

[fkiraly]

Hmm, about the two options:

Use tag to record this functionality

Don't understand this one, could you explain?

Move to a more duck-typing approach. If a component implements get_fitted_params, we get its fitted parameters.

Agreed - that's what sklearn also does on get_params. if hasattr("get_fitted_params") etc.
That would allow interaction with any interface compliant object, although the main doubt I had about it was, what if it has the attr but it does not return a dict, or does not have deep? Additional check on signature inspect?

[RNKuhns]

Hmm, about the two options:

Use tag to record this functionality

Don't understand this one, could you explain?

Realized after re-reading that it was confusing. I was specifically thinking about self.GET_FITTED_PARAMS_NESTING. That is a class variable in BaseEstimator, but that actually seems like it should be a tag or possibly config.

[RNKuhns]

Also -- on line 976 there is a call to get_fitted_params if the component is a BaseEstimator. Shouldn't the deep call get passed down there? If there is an estimator nested in an estimator that is nested in an estimator, I'm guessing we'd want to pass the deep parameter down a level in the call, and gather all the fitted parameters, right?

[RNKuhns]

@fkiraly I posted 2 comments. The first one could be handled in a follow-on PR, since it doesn't deal specifically with this PR's changes. But I think it makes sense to be handled using that system.

The second comment deals with functionality in this PR. Want to make sure that wasn't a missed update. Seems like a spot where we want to pass the deep parameter down to the call of that functionality on a BaseEstimator component.

[fkiraly]

Also -- on line 976 there is a call to get_fitted_params if the component is a BaseEstimator. Shouldn't the deep call get passed down there? If there is an estimator nested in an estimator that is nested in an estimator, I'm guessing we'd want to pass the deep parameter down a level in the call, and gather all the fitted parameters, right?

Strictly speaking no, it wasn't logically wrong, because the False case is treated on line 962/963 by an early return, so if we arrive there we already know that deep is True (which is the default).

However, I agree that it is more error prone in changes, e.g., to the default of get_fitted_params, or moving of paragraphs, so I've added the argument for safety.

The test that certified that "all was good" is the line assert set(comp_f_params_shallow) == {"foo"} in test_get_fitted_params.

[fkiraly]

Realized after re-reading that it was confusing. I was specifically thinking about self.GET_FITTED_PARAMS_NESTING. That is a class variable in BaseEstimator, but that actually seems like it should be a tag or possibly config.

I see, thanks for explaining.

I would agree that it should be a tag or config. I would tend towards a config, since it is not a "property of the estimator" but behaviour modifying in a way that a user or interface may want to repeat with different values.

[fkiraly]

(comments are addressed - no.1 by explanation & change in code; no.2 by saying, let's do this once we have config interface if you agree)

[RNKuhns]

(comments are addressed - no.1 by explanation & change in code; no.2 by saying, let's do this once we have config interface if you agree)

Totally agree on that. I'll make an issue about that.

[RNKuhns]

This LGTM. Will merge once tests have run and passed.