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

[Core] Possible issue with core.py #258

Open
mnajaria opened this issue Dec 20, 2023 · 4 comments
Open

[Core] Possible issue with core.py #258

mnajaria opened this issue Dec 20, 2023 · 4 comments
Labels

Comments

@mnajaria
Copy link

What happened + What you expected to happen

It seems that in the following link we should replace the Y_df with Y_hat_df. In core.py it should change for both balanced (line 278) and imbalanced (line 80).

current code line 278
y_hat_insample = Y_df[model_name].values.reshape(len(S_df), -1).astype(np.float32)
we should have
y_hat_insample = Y_hat_df[model_name].values.reshape(len(S_df), -1).astype(np.float32)
link to the code:
https://github.com/Nixtla/hierarchicalforecast/blob/0b853e511d43fd26463e906e4f2d39014c540239/hierarchicalforecast/core.py#L278C32-L278C32

Versions / Dependencies

0.4.1

Reproduction script

In colab example below if we add MinTrace(method='mint_shrink', nonnegative=True) to reconcilers it will generate error. I fixed it in local by changing Y_df to Y_hat_df
https://colab.research.google.com/github/nixtla/hierarchicalforecast/blob/main/nbs/examples/TourismSmall.ipynb

Issue Severity

Medium: It is a significant difficulty but I can work around it.

@mnajaria mnajaria added the bug label Dec 20, 2023
@mnajaria mnajaria changed the title [<Library component: Core>] Possible issue with core.py [Core] Possible issue with core.py Dec 20, 2023
@jmoralez
Copy link
Member

Hey @mnajaria, thanks for using hierarchicalforecast. That uses the in-sample predictions, so it's ok to be Y_df. I believe that example should compute the in-sample predictions as is done here, but @AzulGarza is the one who knows for sure.

@mnajaria
Copy link
Author

@jmoralez Thanks for your reply and the link you shared.
I received error when I tried it with Y_df and changing it to Y_hat_df didn't generate errors. I will check again to see what the issue is.

@jmoralez
Copy link
Member

The issue is that it'll look for the model columns in Y_df, so providing the in-sample predictions there would fix it. I'm not sure, however, if that's the expected behavior, let's wait for @AzulGarza.

@mnajaria
Copy link
Author

mnajaria commented Jan 5, 2024

If it is Y_hat_insample, shouldn't it be the prediction on train data? Here instead it uses the train Y_df instead of train Y_hat_df.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants