ellisp / forecastHybrid

Convenient functions for ensemble forecasts in R combining approaches from the {forecast} package
GNU General Public License v3.0
79 stars 23 forks source link

Update accuracy formals for upcoming forecast package release #90

Closed mitchelloharawild closed 4 years ago

mitchelloharawild commented 4 years ago

For better consistency with the signature of fabletools::accuracy() and other generics, the forecast::accuracy() generic will be updated in the next release (https://github.com/robjhyndman/forecast/commit/6b9ab090cd01062460c285d17dd6bc6d6d310a48).

This PR updates accuracy methods to use this updated generic signature.

mitchelloharawild commented 4 years ago

Changes to the forecast package in the next version will cause check issues with packages that define methods for accuracy(). There is no time pressure on releasing this version of forecast, but I do think that packages should be submitted at similar times.

On Tue., 25 Feb. 2020, 12:32 David Shaub, notifications@github.com wrote:

@dashaub commented on this pull request.

@mitchelloharawild https://github.com/mitchelloharawild

Thanks for the update; looks good to me. Is this blocking release of forecast 8.12? Will we have to coordinate a simultaneous new CRAN release?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ellisp/forecastHybrid/pull/90?email_source=notifications&email_token=AD3BJF6G57SDWKMCHSN2WBDRESGMNA5CNFSM4KVBWFRKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWX6XPQ#pullrequestreview-363850686, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD3BJFYUQ6BEW6H2UCQSDGTRESGMNANCNFSM4KVBWFRA .

dashaub commented 4 years ago

Right, so if I didn't upload a new CRAN version of "forecastHybrid", wouldn't that block your release of "forecast"? But I can't make a new release yet since this change requires a version of "forecast" that does not exist; we have a circular dependency problem.

At least this is what I've experienced in the past when releasing a CRAN package where the new version breaks a dependency package; they generally block the release until the dependency package is working too.

If this is the case we might want to do it in this order:

  1. Make this change, but without the the version bump requirement of "forecast" 8.12
  2. Assuming "forecastHybrid" still builds fine (not sure, since right now CI fails on installing the nonexistent "forecast" 8.12), make a CRAN "forecastHybrid" release.
  3. The "forecast" package is now unblocked. Make any new releases at your leisure.
mitchelloharawild commented 4 years ago

I've been having some luck with CRAN lately using simultaneous releases, mentioning the simultaneity in the cran-comments.

But yes, it will block forecast and have a circular dependency. I can set a remotes field for dev forecast if you'd like to test changes using your CI.

On Wed., 26 Feb. 2020, 01:14 David Shaub, notifications@github.com wrote:

Right, so if I didn't upload a new CRAN version of "forecastHybrid", wouldn't that block your release of "forecast"? But I can't make a new release yet since this change requires a version of "forecast" that does not exist; we have a circular dependency problem.

At least this is what I've experienced in the past when releasing a CRAN package where the new version breaks a dependency package; they generally block the release until the dependency package is working too.

If this is the case we might want to do it in this order:

  1. Make this change, but without the the version bump requirement of "forecast" 8.12
  2. Assuming "forecastHybrid" still builds fine (not sure, since right now CI fails on installing the nonexistent "forecast" 8.12), make a CRAN "forecastHybrid" release.
  3. The "forecast" package is now unblocked. Make any new releases at your leisure.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ellisp/forecastHybrid/pull/90?email_source=notifications&email_token=AD3BJF5FF5QT3UI5SLASQF3REU7WPA5CNFSM4KVBWFRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM4SNII#issuecomment-590948001, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD3BJF5ONSLLT7JTW46P443REU7WPANCNFSM4KVBWFRA .

mitchelloharawild commented 4 years ago

We're preparing a minor release of forecast within the next week which will include the changes to accuracy() formals. Is it possible to also release a patch for forecastHybrid to go along with this submission?

dashaub commented 4 years ago

@mitchelloharawild Yes, I'll be ready to push a new version right after the new "forecast" goes up.

mitchelloharawild commented 4 years ago

Great, thanks. I'll let you know when forecast is submitted to CRAN.

mitchelloharawild commented 4 years ago

@dashaub forecast has been submitted to CRAN and passed automatic checks.