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

scale_by_forecaster behavior #460

Merged
merged 4 commits into from Feb 23, 2021
Merged

scale_by_forecaster behavior #460

merged 4 commits into from Feb 23, 2021

Conversation

dajmcdon
Copy link
Collaborator

In the R check, there are a few warnings that this avoids.

  • But more importantly, for some reason, scaling seems to ignore any columns that come after the base_forecaster in the df_list. So we move it to the end. I don't understand why this is the case.

@dajmcdon dajmcdon requested review from sgsmob and JedGrabman and removed request for jacobbien February 22, 2021 17:11
@huisaddison huisaddison mentioned this pull request Feb 22, 2021
Open
sgsmob
sgsmob approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@sgsmob sgsmob left a comment

Choose a reason for hiding this comment

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

I don't understand why this is necessary but it looks like it should be safe

JedGrabman
JedGrabman approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@JedGrabman JedGrabman left a comment

Choose a reason for hiding this comment

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

Also not sure why this changes things, but no objection if it fixes the issue.

One minor comment, but don't need to re-review.

R-packages/evalcast/R/scale_by_forecaster.R Outdated Show resolved Hide resolved
@dajmcdon dajmcdon merged commit 985c232 into evalcast-killcards Feb 23, 2021
@dajmcdon dajmcdon deleted the evalcast-djm branch February 23, 2021 01:01
@dajmcdon dajmcdon mentioned this pull request Feb 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgsmob sgsmob sgsmob approved these changes

@JedGrabman JedGrabman JedGrabman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dajmcdon @sgsmob @JedGrabman