Skip to content

Fix #1071: Rename forecast_sample_multivariate class to match constructor#1072

Merged
nikosbosse merged 10 commits intomainfrom
issue-1071-fix-multivariate-class-naming
Feb 15, 2026
Merged

Fix #1071: Rename forecast_sample_multivariate class to match constructor#1072
nikosbosse merged 10 commits intomainfrom
issue-1071-fix-multivariate-class-naming

Conversation

@seabbs-bot
Copy link
Collaborator

@seabbs-bot seabbs-bot commented Feb 12, 2026

Summary

  • Fixes naming inconsistency where class forecast_sample_multivariate did not match constructor as_forecast_multivariate_sample() (word order mismatch)
  • This caused transform_forecasts(append = TRUE) to fail on multivariate sample forecasts because it dynamically constructed as_forecast_sample_multivariate (wrong name) from the class
  • The mismatch was introduced in commit f017891 when the constructor was renamed per review feedback but the class name was not updated

Changes

  • Class renamed from forecast_sample_multivariate to forecast_multivariate_sample
  • Backwards compatibility: old class name retained in class vector during deprecation so inherits(x, "forecast_sample_multivariate") still works
  • Deprecated: is_forecast_sample_multivariate() in favour of is_forecast_multivariate_sample()
  • S3 method aliases: deprecated aliases for score, get_metrics, and assert_forecast methods retained
  • get_forecast_type(): now takes first matching forecast_* class to handle dual-class deprecation period
  • joint_across made optional: when data already has .mv_group_id (needed for transform_forecasts object reconstruction)
  • Example data regenerated with new class structure

Test plan

  • All 56 multivariate sample + transform tests pass
  • Full test suite passes (687/688, 1 pre-existing unrelated snapshot failure)
  • transform_forecasts(append = TRUE) now works on multivariate sample forecasts
  • Deprecated is_forecast_sample_multivariate() emits warning and still returns correct result
  • Both old and new class names present in class vector for backwards compat

Closes #1071

This was opened by a bot. Please ping @seabbs for any questions.

seabbs-bot and others added 3 commits February 12, 2026 18:17
Rename class from forecast_sample_multivariate to
forecast_multivariate_sample to match the constructor function
as_forecast_multivariate_sample(). The old class name is retained
in the class vector during the deprecation period so
inherits(x, "forecast_sample_multivariate") still works.

This fixes transform_forecasts(append = TRUE) which previously
failed because it dynamically constructed the wrong constructor
name from the class.

Other changes:
- is_forecast_sample_multivariate() deprecated in favour of
  is_forecast_multivariate_sample()
- Deprecated S3 method aliases for score, get_metrics, and
  assert_forecast retained for backwards compatibility
- get_forecast_type() now takes the first matching forecast_*
  class to handle the dual-class deprecation period
- joint_across made optional in the constructor when data
  already has .mv_group_id (needed for transform_forecasts
  reconstruction)

Closes #1071
Ensures get_forecast_type() errors clearly if a class matches
the forecast_ prefix but has no type suffix.
@seabbs-bot
Copy link
Collaborator Author

Note on deprecation wrappers: Since multivariate scoring hasn't been included in a CRAN release yet (it was merged after v2.1.2), no external users have the old forecast_sample_multivariate class name. The deprecation wrappers (is_forecast_sample_multivariate(), the deprecated S3 method aliases, and the old class in the class vector) are therefore not strictly necessary for backwards compatibility right now.

Options:

  1. Keep them as-is for safety during the development cycle, then remove before the next CRAN release
  2. Remove them now since no one outside of development is using the old names

I'd suggest option 1 (keep for now, remove before CRAN) as the safest approach since it avoids breaking any in-progress work that might reference the old names (e.g. hubEvals development), but we should clean them up before going to CRAN to avoid shipping deprecated code that was never public.

This was opened by a bot. Please ping @seabbs for any questions.

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This looks good to me. As noted I think we probably don't need to push to CRAN with the mismatched naming but can drop the depreciations instead (having some warning of this does seem like a good idea for those living on the bleeding edge).

The linter changed paste(..., collapse = ", ") to toString() in
snapshots but not in the test code, causing CI failures.
@seabbs seabbs requested review from nikosbosse and sbfnk February 12, 2026 18:43
Aligns test code and snapshots to use toString() consistently,
fixing CI snapshot mismatches.
@codecov
Copy link

codecov bot commented Feb 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.85%. Comparing base (fe9c9f0) to head (2243f77).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1072      +/-   ##
==========================================
+ Coverage   97.83%   97.85%   +0.02%     
==========================================
  Files          35       35              
  Lines        1845     1865      +20     
==========================================
+ Hits         1805     1825      +20     
  Misses         40       40              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Cover the cli_abort when joint_across is NULL and .mv_group_id is
absent from the data.
@seabbs
Copy link
Contributor

seabbs commented Feb 12, 2026

Some linting issues to clean up

@nikosbosse
Copy link
Collaborator

Thanks @seabbs

@nikosbosse nikosbosse merged commit 2164ca4 into main Feb 15, 2026
11 checks passed
@nikosbosse nikosbosse deleted the issue-1071-fix-multivariate-class-naming branch February 15, 2026 16:18
nikosbosse added a commit that referenced this pull request Mar 1, 2026
…iate

Since multivariate scoring hasn't been in a CRAN release yet, the
deprecation wrappers added in #1072 are not needed for backwards
compatibility. This removes:

- The deprecated class name "forecast_sample_multivariate" from the
  class vector (it was appended after "forecast" for compat)
- assert_forecast.forecast_sample_multivariate S3 method alias
- is_forecast_sample_multivariate() deprecated function
- score.forecast_sample_multivariate S3 method alias
- get_metrics.forecast_sample_multivariate S3 method alias

Also regenerates example data and documentation to reflect the
clean class vector.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

Fix naming inconsistency: forecast_sample_multivariate class vs as_forecast_multivariate_sample constructor

3 participants