Skip to content

[BUG] ensure BaseMetaObject resets at set_params; add 'test_set_params_resets_fitted_state' test to trigger error#467

Merged
fkiraly merged 5 commits intosktime:mainfrom
SimonBlanke:bugfix/reset_at_set_params
Nov 15, 2025
Merged

[BUG] ensure BaseMetaObject resets at set_params; add 'test_set_params_resets_fitted_state' test to trigger error#467
fkiraly merged 5 commits intosktime:mainfrom
SimonBlanke:bugfix/reset_at_set_params

Conversation

@SimonBlanke
Copy link
Contributor

@SimonBlanke SimonBlanke commented Nov 8, 2025

Reference Issues/PRs

fixes #412

What does this implement/fix? Explain your changes.

This PR adds a test to trigger the bug described in #412. If desired this PR will also add a fix to the bug.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions
  • I've reviewed the project documentation on contributing
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
    the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added (see documentation standards)
  • New public functionality has been added to the API Reference

@SimonBlanke
Copy link
Contributor Author

SimonBlanke commented Nov 8, 2025

One problem in #413 is, that reset is called multiple times in some instances. It would be better to always have one reset. I attempt to fix this problem and the original issue in this commit 07044cb

@codecov
Copy link

codecov bot commented Nov 8, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.28%. Comparing base (306958d) to head (67d6ad4).
⚠️ Report is 161 commits behind head on main.

Files with missing lines Patch % Lines
skbase/base/_meta.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
+ Coverage   85.07%   86.28%   +1.21%     
==========================================
  Files          45       50       +5     
  Lines        3015     3602     +587     
==========================================
+ Hits         2565     3108     +543     
- Misses        450      494      +44     

☔ 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.

@SimonBlanke
Copy link
Contributor Author

The tests I added to trigger the bug described in the issue now pass after implementing my fix.

@SimonBlanke SimonBlanke marked this pull request as ready for review November 8, 2025 11:17
@SimonBlanke SimonBlanke changed the title add 'test_set_params_resets_fitted_state' test to trigger error from … [BUG] fixes #412; add 'test_set_params_resets_fitted_state' test to trigger error Nov 8, 2025
@fkiraly fkiraly added the bug Something isn't working label Nov 9, 2025
@fkiraly fkiraly changed the title [BUG] fixes #412; add 'test_set_params_resets_fitted_state' test to trigger error [BUG] ensure BaseMetaObject resets at set_params; add 'test_set_params_resets_fitted_state' test to trigger error Nov 9, 2025
Copy link
Contributor

@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.

Brilliant!

I now understand that the return self call is the culprit.

This is unfortunate, I did spot it as an inconsistency and indeed it actually turned out (by your diagnosis) that it is the problem here.

If the clause if params is None: return self would not happen, would the issue not have happened? That is a strong indication that this special case caused a problem.

Either way, that is out of scope for this PR, but perhaps we should open an issue on whether this needs a change?

@fkiraly
Copy link
Contributor

fkiraly commented Nov 14, 2025

issue opened here: #469

@fkiraly fkiraly merged commit 477ac96 into sktime:main Nov 15, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] BaseMetaObject does not reset at set_params call

2 participants