[ENH] Migrate base class registry to a class based structure and lookup logic#925
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the skpro base class registry to a class-based structure (following sktime's pattern), as requested in issue #898. It introduces _base_classes.py with scitype classes that register name, description, base class reference, and test class reference, and refactors the test class registry and lookup logic to dynamically use this new structure.
Changes:
- Added
skpro/registry/_base_classes.pywith class-based scitype definitions (object, estimator, regressor_proba, distribution, metric, metric_distr, datatype, converter, datatype_example) and utility functions for registry lookup. - Refactored
test_class_register.py(moved fromskpro/tests/toskpro/registry/) to dynamically build the test class registry from_base_classesinstead of hardcoding imports, and added deduplication logic. - Fixed a latent bug in
_scitype.pywherescitypeswas unset whentag_typewasNone, and updated_lookup.pyto derive valid object type strings from the new registry.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
skpro/registry/_base_classes.py |
New module with class-based scitype registry and utility functions |
skpro/registry/test_class_register.py |
Refactored to dynamically build registry from _base_classes |
skpro/registry/__init__.py |
Exports new registry functions and test class utilities |
skpro/registry/_lookup.py |
Uses get_obj_scitype_list() for valid object type strings |
skpro/registry/_scitype.py |
Bug fix: handles None tag_type case |
skpro/registry/_tags.py |
Documentation update |
skpro/registry/tests/test_registry_logic.py |
New tests for the registry logic |
skpro/utils/estimator_checks.py |
Updated import path |
skpro/tests/test_switch.py |
Updated import path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
Hi @fkiraly , I've submitted this PR to address the circular dependencies in |
fixing this : created a circular dependency by exposing test utilities in your main __init__.py Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…d_tree_cached when mode=class
|
Hi @fkiraly , would appreciate your feedback on this pull request |
fkiraly
left a comment
There was a problem hiding this comment.
Looks great, I left a small number of comments above.
|
Hi @fkiraly , sorry could not look into it. Was travelling out. Thank you for the fix. |
Reference Issues/PRs
Fixes #898
What does this implement/fix? Explain your changes.
This PR refactors the
skpro/registrymodule to improve maintainability and resolve potential circular import issues. Key changes include:Extracted base classes from the registry logic into a new dedicated module,
skpro/registry/_base_classes.py.Improved the registry structure by separating core lookup logic,
scitypeinference, and tag management.Lazy Loading: Applied the
sktime/skprostandard pattern for soft dependencies to ensure these are only imported when required, preventing premature dependency loading and circular imports.Moved
skpro/tests/test_class_register.pytoskpro/registry/test_class_register.pyto better align tests with the source code structure.Note: Also improves modularity and resolves circular dependency patterns in the registry
Did you add any tests for the change?
Yes
skpro/registry/tests/test_registry_logic.pyto verify the new modular structure.Any other comments?
I have modularized the registry to improve maintainability and decouple core logic. I would appreciate your feedback on two points:
Does the extraction of base classes into
skpro/registry/_base_classes.pyalign with your long-term vision?Are there specific registry edge cases I should include in the new
test_registry_logic.py?Note: I encountered a local AssertionError in
test_proba_basic.pydue tomatplotlibversioning in Python 3.13. My code changes are functionally verified and pass theregistry/lazy-loadsanity checks, but I wanted to flag this in case it affects CI.