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

Fixed small bug in performance_metrics/forecasting/_functions.py #422

Merged
merged 3 commits into from Oct 12, 2020

Conversation

krumeto
Copy link
Contributor

@krumeto krumeto commented Oct 10, 2020

Reference Issues/PRs

There is no currently open issue or PR on this bug

What does this implement/fix? Explain your changes.

In the mase_loss function, there is a check if the training set is prior to test set. In the latest sktime version, the check was comparing y_train.index.max() >= y_test.min() , rather than y_train.index.max() >= y_test.index.min(). In addition, the ValueError raised a comment which was referring to y_pred, while the check performed was against y_test. Both were straightforward fixes.

Does your contribution introduce a new dependency? If yes, which one?

No new dependency.

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

Any other comments?

…to the check if training set is prior to test set
riteshahuja13
riteshahuja13 previously approved these changes Oct 12, 2020
Copy link

@riteshahuja13 riteshahuja13 left a comment

Choose a reason for hiding this comment

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

This is causing the function to break in my use case as well. #426
With these changes it works well.

@mloning
Copy link
Contributor

mloning commented Oct 12, 2020

Thanks for the fix @krumeto and the feedback @riteshahuja13! Synced with master and will merge once all tests pass!

@mloning mloning merged commit c5a559d into sktime:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants