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.

Sign up for GitHub

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

Jump to bottom

[ENH] NeuralForecastRNN should auto-detect freq #6003

Closed
fkiraly opened this issue Feb 25, 2024 · 3 comments · Fixed by #6039
Closed

[ENH] NeuralForecastRNN should auto-detect freq #6003

fkiraly opened this issue Feb 25, 2024 · 3 comments · Fixed by #6039
Labels
enhancement Adding new functionality module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting

Comments

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 25, 2024 •
edited

Right now, a freq argument needs to be passed to the constructor of NeuralForecastRNN, and it will raise exception if not.

To my understanding, this must always be identical with the freq of the data (as pandas coerced),
Therefore, this information is available in fit, so why not have a default setting of "auto", in which case fh.freq is used? This would also be deprecation safe, as an arg without default would get one.
@fkiraly fkiraly added module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting enhancement Adding new functionality labels Feb 25, 2024
@fkiraly
Copy link
Collaborator Author

fkiraly commented Feb 25, 2024

FYI @yarnabrina

@yarnabrina
Copy link
Collaborator

I replied to your comment in the PR, but I think it got lost in later conversations.

I'm not sure if I understand how to use fh.freq always. If user passes a relative fh, say [1, 6, 12], will that object always have a freq attribute? May be that is handled in BaseForecaster or in ForecastingHorizon, but I'm not very familiar with those.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Feb 25, 2024

ForecastingHorizon is designed to behave "greedy for freq", i.e., all objects it interacts with it queries for freq and sets to itself if not already set.

As a consequence, once _fit is reached and fh has been passed, it has a freq if the data had the attribute.

Therefore, trying to get freq from fh should be "safe" in a case where fh was passed. If it does not have freq, it means it cannot be inferred from the data.

In consequence, a sensible "auto" setting could get it from fh, and when that is not available, it could complain to the user.

geetu040 added a commit to geetu040/sktime that referenced this issue Mar 1, 2024
@geetu040
[ENH] NeuralForecastRNN should auto-detect freq (sktime#6003)
ad840de 
Enhances `NeuralForecastRNN` to interpret `freq` from `ForecastingHorizon` when passed as `"auto"`
@geetu040 geetu040 mentioned this issue Mar 1, 2024
Merged
3 tasks
fkiraly pushed a commit that referenced this issue Mar 20, 2024
@geetu040
[ENH] NeuralForecastRNN should auto-detect freq (#6039)
d19fda1 
Enhances `NeuralForecastRNN` to interpret `freq` from
`ForecastingHorizon` when passed as `"auto"`
<!--
Welcome to sktime, and thanks for contributing!
Please have a look at our contribution guide:
https://www.sktime.net/en/latest/get_involved/contributing.html
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.

Please use keywords (e.g., Fixes) to create link to the issues or pull
requests
you resolved, so that they will automatically be closed when your pull
request
is merged. See
https://github.com/blog/1506-closing-issues-via-pull-requests.
If no issue exists, you can open one here:
https://github.com/sktime/sktime/issues
-->
Fixes #6003.

#### What does this implement/fix? Explain your changes.
<!--
A clear and concise description of what you have implemented.
-->
The `NeuralForecastRNN` constructor previously required a `freq`
argument, which is now proposed to default to `"auto"` in which case it
interprets `freq` from `ForecastingHorizon`, leveraging `fh.freq` in the
`fit` method.

#### What should a reviewer concentrate their feedback on?

<!-- This section is particularly useful if you have a pull request that
is still in development. You can guide the reviews to focus on the parts
that are ready for their comments. We suggest using bullets (indicated
by * or -) and filled checkboxes [x] here -->
I have run the tests with the updated estimator
```py
results = check_estimator(NeuralForecastRNN) # All tests PASSED!
```
`freq` can now be passed like this:
```py
y, X = load_longley()
y_train, y_test, X_train, X_test = temporal_train_test_split(y, X, test_size=4)

model = NeuralForecastRNN(
	"auto",	# interprets to be "A-DEC"
	futr_exog_list=["ARMED", "POP"], max_steps=5)

model.fit(y_train, X=X_train, fh=[1, 2, 3, 4])

model.predict(X=X_test)
# Seed set to 1
# 1959    66241.984375
# 1960    66700.132812
# 1961    66550.195312
# 1962    67310.007812
# Freq: A-DEC, Name: TOTEMP, dtype: float64
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Adding new functionality module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[ENH] NeuralForecastRNN should auto-detect freq geetu040/sktime
2 participants
@fkiraly @yarnabrina