Skip to content

[MNT] Refactor CyclicBoosting to eliminate feature validation code duplication#917

Merged
fkiraly merged 5 commits intosktime:mainfrom
MayankSharma-2812:refactor/cyclic-boosting-feature-validation
Mar 16, 2026
Merged

[MNT] Refactor CyclicBoosting to eliminate feature validation code duplication#917
fkiraly merged 5 commits intosktime:mainfrom
MayankSharma-2812:refactor/cyclic-boosting-feature-validation

Conversation

@MayankSharma-2812
Copy link
Contributor

@MayankSharma-2812 MayankSharma-2812 commented Mar 14, 2026

Reference Issues/PRs
Fixes #916

What does this implement/fix? Explain your changes.
This refactoring eliminates code duplication in CyclicBoosting by extracting the repeated feature validation logic into a single helper method _validate_feature_groups(). The change:

Adds helper method _validate_feature_groups(X) that validates all feature groups exist in X columns
Replaces duplicated code in 5 methods with single helper call:
_fit()
_predict()
_predict_proba()
_predict_interval()
_predict_quantiles()
Maintains identical behavior - same error messages and validation logic
Adds a test to ensure missing feature groups raise the expected error.
Does your contribution introduce a new dependency? If yes, which one?
No new dependencies introduced.

What should a reviewer concentrate their feedback on?
Verify that the helper method logic is identical to the original duplicated code
Ensure all 5 methods correctly call the helper method
Check that error messages remain unchanged
Confirm the new test provides adequate coverage
Did you add any tests for the change?
Yes. Added test_cyclic_boosting_missing_feature_validation() to skpro/regression/tests/test_cyclic_boosting.py which:

Tests error messages for missing features
Validates tuple feature groups work correctly
Ensures the helper method is called consistently across all prediction methods
Any other comments?
This is a pure maintenance refactoring that improves code maintainability without changing any external behavior. The fix follows DRY principles and reduces future maintenance burden.

PR checklist
For all contributions
I've added myself to the list of contributors with any new badges I've earned :-)
The PR title starts with [MNT] - maintenance refactoring

@MayankSharma-2812
Copy link
Contributor Author

All existing tests pass locally and the refactor preserves the original validation behavior.
Happy to adjust implementation if maintainers prefer a different structure.

@fkiraly fkiraly added enhancement module:regression probabilistic regression module labels Mar 14, 2026
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Thanks!

Although, feature validation is not required in the predictmethods, because they are always called after fit - can you remove it from there?

@MayankSharma-2812
Copy link
Contributor Author

Thanks!

Although, feature validation is not required in the predictmethods, because they are always called after fit - can you remove it from there?

Thanks for the suggestion! That makes sense.

I removed the validation calls from the predict methods and kept the
validation only in _fit(). The full test suite passes locally.

Please let me know if anything else should be adjusted.

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 15, 2026

code quality checks are failing, look at the dev guide or use pre-commit

@MayankSharma-2812 MayankSharma-2812 force-pushed the refactor/cyclic-boosting-feature-validation branch from 84615a8 to 885fac6 Compare March 16, 2026 03:56
@MayankSharma-2812
Copy link
Contributor Author

code quality checks are failing, look at the dev guide or use pre-commit

Done. The CI issue was related to black formatting in test_proba_basic.py. But now its resolved.

Please lmk for any further adjustments

@fkiraly fkiraly merged commit 1a0b918 into sktime:main Mar 16, 2026
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement module:regression probabilistic regression module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENH] Code duplication in CyclicBoosting feature validation logic

2 participants