search

robjhyndman / forecast

Forecasting Functions for Time Series and Linear Models
http://pkg.robjhyndman.com/forecast
1.11k stars 341 forks source link

warnings for partial match #841

Closed EmilRehnberg closed 4 years ago

EmilRehnberg commented 4 years ago

Hi! This package gives plenty warnings regarding partial matches (while accessing names).

Is it ok if I take a stab at fixing this and submit a PR?

robjhyndman commented 4 years ago

Under what circumstances? The package passes all cran checks and unit tests without warnings

EmilRehnberg commented 4 years ago

on 8.11 the tests in tests/testthat/test-tbats.R gives me 6 warnings.

Looking through my options, I got warnPartialMatchDollar = TRUE.

Do you think it's reasonable to have the forecast package pass tests with the warnPartialMatch flags set to TRUE?

the option flags are

robjhyndman commented 4 years ago

There are no warnings on my computer, or on CRAN, or on Travis. See https://travis-ci.org/robjhyndman/forecast/jobs/648131680 or https://cran.r-project.org/web/checks/check_results_forecast.html. So whatever is going on, I suspect it has something to do with your packages or settings.

mitchelloharawild commented 4 years ago

With these optional warnings set I get many partial matches throughout the package. For test-tbats.R, I get:

─────────────────────────────────────────────────────────────────────────────────
✓ |  21   6 1 | Tests on tbats() functions [12.9 s]
─────────────────────────────────────────────────────────────────────────────────
test-tbats.R:37: warning: Test tbats() and forecasts
partial match of 'gamma.one.v' to 'gamma.one.values'

test-tbats.R:37: warning: Test tbats() and forecasts
partial match of 'gamma.two.v' to 'gamma.two.values'

test-tbats.R:45: warning: Test tbats() and forecasts
partial match of 'gamma.one.v' to 'gamma.one.values'

test-tbats.R:45: warning: Test tbats() and forecasts
partial match of 'gamma.two.v' to 'gamma.two.values'

test-tbats.R:45: warning: Test tbats() and forecasts
partial match of 'gamma.one.v' to 'gamma.one.values'

test-tbats.R:45: warning: Test tbats() and forecasts
partial match of 'gamma.two.v' to 'gamma.two.values'

Avoiding partial matching is the right thing to do, and so if you're interested in making a PR for this I'd happly accept it.

EmilRehnberg commented 4 years ago

@mitchelloharawild thanks for that! I'll submit something in due time.

EmilRehnberg commented 4 years ago

Hi @mitchelloharawild Looks like you started on this, do you want to continue? I also started adding some fixes.

mitchelloharawild commented 4 years ago

Happy for you to continue with the fixes, I wanted to add the partial matches to the check requirements. From the check warnings it looks like there are a lot of partial matching, so of course there is no pressure for you to fix all of them. Submit a PR fixing however many partial matches you'd like and then I'll fix the rest.

EmilRehnberg commented 4 years ago

I think the merged code closes this issue.