Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNT] address pd.DataFrame.groupby(axis=1) deprecation in EnsembleForecaster #5707

Merged
merged 4 commits into from Jan 9, 2024

Conversation

ninedigits
Copy link
Contributor

@ninedigits ninedigits commented Jan 6, 2024

@fkiraly fkiraly added maintenance Continuous integration, unit testing & package distribution module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting labels Jan 6, 2024
@fkiraly
Copy link
Collaborator

fkiraly commented Jan 6, 2024

You should also update all-contributorsrc with your badges!
maintenance I suppose.

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 6, 2024

Your update is failing linting (code formatting) checks - perhaps you may like to split the one-liner into multiple lines to avoid too many brackets and indents.

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 6, 2024

PS, how about this (different from the other code in the issue):

y_pred = y_pred.T.groupby(level=1).agg( 
     _aggregate, self.aggfunc, self.weights 
 ).T

@ninedigits
Copy link
Contributor Author

ninedigits commented Jan 7, 2024

PS, how about this (different from the other code in the issue):

y_pred = y_pred.T.groupby(level=1).agg( 
     _aggregate, self.aggfunc, self.weights 
 ).T

In order to get this to work, the input data into _aggregate also needs to be transformed.

y_pred = y_pred.T.groupby(level=1).agg(
    lambda y, aggfunc, weights: _aggregate(y.T, aggfunc, weights), 
    self.aggfunc, 
    self.weights
).T

This passes the unit tests and avoids the FutureWarning, and it's similar to the original code. We could keep this line the same

lambda y, aggfunc, weights: _aggregate(y.T, aggfunc, weights)

Or define it as a wrapper somewhere else:

def _aggregate_transform(y, aggfunc, weights):
    return _aggregate(y.T, aggfunc, weights)

What do you think?

Edit:
I have a unit test that I wrote to see if the future warning is triggered. Not sure it's appropriate to include in the codebase since a different future warning might be triggered in the future.

@pytest.mark.parametrize(
    "forecasters",
    [
        [("trend", PolynomialTrendForecaster()), ("naive", NaiveForecaster())],
    ],
)
def test_ensemble_pandas_future_warn(forecasters):
    """Raises exception if FutureWarning triggered within EnsembleForecaster"""
    y = make_forecasting_problem()
    with pytest.warns(None) as record:

        forecaster = EnsembleForecaster(forecasters)
        forecaster.fit(y, fh=[1, 2, 3])
        _ = forecaster.predict()
        for warning in record:
            if isinstance(warning.message, FutureWarning):
                raise AssertionError("FutureWarning was triggered")

Revising using `frame.T.groupby(level=1)` instead
fkiraly
fkiraly previously approved these changes Jan 7, 2024
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that works!

Re warning, we monitor for FutureWarning-s regularly, but addressing them is a matter of maintainer time, so this is very much appreciated! They are listed at the end of diagnostic runs (you can look at "details" and the CRON job)

@ninedigits
Copy link
Contributor Author

Looks like it still failed the linting steps. Should I revise or are we good to go?

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 8, 2024

Should I revise or are we good to go?

Please revise, the tests should all pass.

Here's a guide on setting up linting checks on your computer:
https://www.sktime.net/en/stable/developer_guide/coding_standards.html#setting-up-local-code-quality-checks

Let me know if it turns out too fiddly, I can do it for you then.

Reformatted to conform to `black` code format
@ninedigits
Copy link
Contributor Author

Should I revise or are we good to go?

Please revise, the tests should all pass.

Here's a guide on setting up linting checks on your computer: https://www.sktime.net/en/stable/developer_guide/coding_standards.html#setting-up-local-code-quality-checks

Let me know if it turns out too fiddly, I can do it for you then.

Thanks, I submitted the latest revision after running the black code reformatting steps.

@fkiraly fkiraly merged commit 5ab2199 into sktime:main Jan 9, 2024
55 checks passed
@fkiraly fkiraly changed the title [MNT] changes pandas groupby(axis=1) in EnsembleForecaster [MNT] address pd.DataFrame.groupby(axis=1) deprecatio in EnsembleForecaster (#5707) Jan 9, 2024
@fkiraly fkiraly changed the title [MNT] address pd.DataFrame.groupby(axis=1) deprecatio in EnsembleForecaster (#5707) [MNT] address pd.DataFrame.groupby(axis=1) deprecatio in EnsembleForecaster Jan 9, 2024
@fkiraly fkiraly changed the title [MNT] address pd.DataFrame.groupby(axis=1) deprecatio in EnsembleForecaster [MNT] address pd.DataFrame.groupby(axis=1) deprecation in EnsembleForecaster Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Continuous integration, unit testing & package distribution module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MNT] Pandas groupby(axis=1) will be deprecated in a future version, affects EnsembleForecaster
2 participants