[ENH] test that forecasters preserve name attr of pd.Series
#4157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fkiraly
added
module:forecasting
This was referenced Jan 27, 2023
fkiraly
added a commit
that referenced
this pull request
Feb 1, 2023
…4161) This PR collects fixes for a number of forecasters to preserve the `name` attribute in `predict`. See #4144. The issues were found by #4157. Does *not* contain changes to the conversion logic, or tests (e.g., #4157). Testing is via merging this PR into the options dealing with the `name` attribute. Contains fixes for: * `ARDL` * `ARIMA` and `AutoARIMA` * `AutoEnsembleForecaster` * `AutoETS` * `Croston` * `NaiveForecaster` * `OnlineEnsembleForecaster` * reducers * `SquaringResiduals` * `StackingForecaster` * `StatsForecastAutoARIMA` * `STLForecaster` * `ThetaModularForecaster` * `TrendForecaster` and `PolynomialTrendForecaster` As tempting as it might be to add the fix to `BaseForecaster` somewhere in `predict`: whilt it might be DRY-er, I think it would muddle concerns. If at all, this should go in `convert`-like boilerplate rather than in the base class itself, but unclear how that would look like.
fkiraly
added a commit
that referenced
this pull request
Feb 1, 2023
…to/from `pd.DataFrame` and `np.ndarray`, as `Series` scitype (#4150) This PR ensures that the `pd.Series` `name` attribute is preserved when converting to and back from `pd.DataFrame` or `np.ndarray` under the `Series` scitype. Fixes #4144. Currently, the back-conversion intentionally always reset the `name` attribute to `None`, which could lead to unexpected behaviour of some estimators, and user frustration, see #4144 Simply removing the "set to None" breaks other things, in the case of conversion `pd.DataFrame` to `pd.Series` and back. (experimental PR until tests are added and it is ensured that nothing else breaks) Depends on: * #4157 for testing * #4161 for fixing uncovered bugs via #4157
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a test,
test_predict_series_name_preserved, to the forecasting test suite, to test that thenameattribute ofpd.Seriesis preserved infit/predict.A test for detecting the general issue at the root of #4144, which currently was not covered by the forecasting test suite, as the scenarios containing
pd.Serieswith non-Nonenameattribute did not interact with the tests in which the time index and name were checked for consistency via_assert_correct_pred_time_index.If this works, this should surface a number of cases with the problem, among them
NaiveForecaster(where this surfaced through manual testing in the context of #4150 failures)