+
Skip to content

Conversation

sinemkilicdere
Copy link
Contributor

@sinemkilicdere sinemkilicdere commented Jul 21, 2025

Moved the skip test configuration for FCNClassifier to the estimator's _tags as for issue 8515.

  • Removed FCNClassifier from EXCLUDED_TESTS in _config.py
  • Added tests:skip_by_name tag in FCNClassifier class

Towards #8515

Reference Issues/PRs

What does this implement/fix? Explain your changes.

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

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the sktime root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • Optionally, for added estimators: I've added myself and possibly to the maintainers tag - do this if you want to become the owner or maintainer of an estimator you added.
    See here for further details on the algorithm maintainer role.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

@sinemkilicdere sinemkilicdere force-pushed the FCNClassifier-tag-sinem branch from f631c57 to 7f843c1 Compare July 22, 2025 14:10
@fkiraly fkiraly moved this to PR in progress in May - Sep 2025 mentee projects Jul 24, 2025
@fkiraly fkiraly moved this from PR in progress to PR under review in May - Sep 2025 mentee projects Jul 24, 2025
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.

Great! I think you missed an instance of FCNClassifier in _config though - please go through the recipe and check again.

@dtemir
Copy link

dtemir commented Jul 28, 2025

Great! I think you missed an instance of FCNClassifier in _config though - please go through the recipe and check again.

@sinemkilicdere I believe @fkiraly is referring to these lines in the config (line 51 and 54)

"FCNClassifier",
"EditDist",
"CNNClassifier",
"FCNClassifier",

@sinemkilicdere
Copy link
Contributor Author

Great! I think you missed an instance of FCNClassifier in _config though - please go through the recipe and check again.

@sinemkilicdere I believe @fkiraly is referring to these lines in the config (line 51 and 54)

"FCNClassifier",
"EditDist",
"CNNClassifier",
"FCNClassifier",

Thanks Damir. working on now

@fkiraly fkiraly added module:classification classification module: time series classification enhancement Adding new functionality labels Jul 28, 2025
@sinemkilicdere
Copy link
Contributor Author

Great! I think you missed an instance of FCNClassifier in _config though - please go through the recipe and check again.

thanks for review. Issues have been resolved

@sinemkilicdere sinemkilicdere requested a review from fkiraly July 28, 2025 21:05
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, I think there should also be a tests:skip_by_name tag which is missing. I recommend reading through the recipe #8515 again.

@fkiraly fkiraly changed the title Move FCNClassifier test skip rule to estimator tag (fixes #8515) [ENH] Move FCNClassifier test skip rule to estimator tag Aug 1, 2025
@sinemkilicdere sinemkilicdere force-pushed the FCNClassifier-tag-sinem branch from 2bad497 to a184660 Compare August 1, 2025 12:12
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.

Great!

@fkiraly fkiraly merged commit e9cc78a into sktime:main Aug 6, 2025
82 of 84 checks passed
@github-project-automation github-project-automation bot moved this from PR under review to Done in May - Sep 2025 mentee projects Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Adding new functionality module:classification classification module: time series classification

Projects

Development

Successfully merging this pull request may close these issues.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载