Skip to content

[ENH] tag registry upgrade - full tag documentation, user_facing field, env_marker tag - #1183

Open
adity1raut wants to merge 1 commit into
sktime:mainfrom
adity1raut:enh-tag-registry-docs
Open

adity1raut wants to merge 1 commit into
sktime:mainfrom
adity1raut:enh-tag-registry-docs

Conversation

@adity1raut

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #1083.

This PR does the remaining item of #1083, the tag registry and tag documentation. The __post_init__ and __dynamic_tags__ item was done in #1094.

As discussed in the issue, changing all_tags filtering to include "object" and "estimator" tags is left for a separate PR.

What does this implement/fix? Explain your changes.

This PR brings skpro.registry._tags in line with sktime.registry._tags:

  • Tag documentation: every tag class now has a docstring in the sktime format: a one-line summary, then string name, tag category (public / private / extension developer), values, examples and default, then a description. The text describes what each tag does in skpro, checked against the code that reads it, for example:
    • capability:update: update does nothing if the tag is False.
    • distr:paramtype: get_params_df and to_df only work for parametric distributions.
    • distr:measuretype: pdf returns zero for discrete distributions, and pmf returns zero for continuous ones.
    • reserved_params: these parameters are exempt from some constructor and set_params tests.
  • user_facing field: added to _BaseTag and set on every tag, as in sktime.
  • object_type docstring: lists all scitypes, generated from get_obj_scitype_list(return_descriptions=True). This is skipped if docstrings are stripped (python -OO).
  • New env_marker tag (PEP 508): skbase's _check_estimator_deps already reads it (since scikit-base 0.8.2), but it was missing from the registry.
  • Tag metadata fixes:
    • object_type now has type ("list", "str"), because objects can have several types, e.g., ["metric", "metric_distr"]. Before this, check_tag_is_valid rejected list values.
    • The object_type and estimator_type descriptions no longer give 'transformer' as an example, since skpro has no transformer scitype.
    • python_dependencies_alias and estimator_type are documented as legacy tags: no current code reads them.
  • Register construction now runs inside a function. Before, the module-level loop left cl, tag_name, etc. in the module namespace. cl pointed to the last tag class, so y_inner_mtype showed up twice when scanning the module. The register itself was correct, because the scan ran before the loop.
  • Module docstring now explains how to add a tag class (naming convention and _tags fields). It previously said to edit OBJECT_TAG_REGISTER directly, which is outdated.
  • API reference: env_marker added to docs/source/api_reference/tags.rst. Every tag class is now listed there.

The register contents are otherwise unchanged. Compared with main, the only differences are the new env_marker row and the updated object_type and estimator_type rows.

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

No.

What should a reviewer concentrate their feedback on?

  • Whether the tag docstrings describe skpro behaviour correctly.
  • The user_facing value of each tag. I set capability, property and metadata tags to True. The approx_*_spl and bisect_iter tags are also True, as "configuration" tags users may change with set_tags. Packaging, CI and extension developer tags are False.
  • The object_type tag type change to ("list", "str").

Did you add any tests for the change?

Yes, in skpro/registry/tests/test_tags.py:

  • test_tag_class_spec, run for each tag class, checks that:
    • tag_name and short_descr are filled in;
    • the class name equals the tag name with : replaced by __;
    • parent_type values are valid scitypes;
    • short_descr is at most 80 characters;
    • user_facing is a bool;
    • the docstring has the - String name: ... line.
  • test_tag_names_unique checks that no tag name is defined twice. This test found the namespace leak described above.
  • test_object_type_doc_lists_scitypes checks that the generated object_type docstring lists every scitype.
  • test_check_tag_is_valid covers valid and invalid values, including a list-valued object_type.

Checks run locally:

  • pytest skpro/registry: 89 passed.
  • pytest skpro/tests/test_all_estimators.py -k tag: 650 passed.
  • pre-commit hooks pass on the changed files.
  • All tag docstrings parse as reStructuredText with docutils, with no warnings.

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 either [ENH], [MNT], [DOC], or [BUG].

…eld, `env_marker` tag

Upgrades the tag registry in `skpro.registry._tags` to the `sktime` pattern,
as per sktime#1083:

* all tag classes have structured docstrings, following `sktime.registry._tags`:
  string name, tag category, values, examples, default, and description,
  describing the actual behaviour of the tag in `skpro`
* `user_facing` field added to `_BaseTag` and all tags
* `object_type` docstring dynamically lists all scitypes
* new `env_marker` tag (PEP 508), which `skbase`'s `_check_estimator_deps`
  already checks
* `object_type` tag type is now str or list of str, as polymorphic objects exist
* register construction moved into a function, to avoid loop variables
  leaking into the module namespace
* module docstring explains how to add a new tag class
* tests for the tag class specification and `check_tag_is_valid`
@adity1raut

Copy link
Copy Markdown
Contributor Author

@fkiraly this covers the remaining tag registry item from #1083. The __post_init__ / __dynamic_tags__ part already went in with #1094.

What changed:

  • every tag in skpro.registry._tags now has a full docstring in the sktime format, written against what the tag actually does in skpro
  • user_facing field added to all tags, and the object_type docstring now lists the scitypes automatically
  • new env_marker tag, since skbase's _check_estimator_deps already reads it
  • object_type now accepts a list, so check_tag_is_valid no longer rejects polymorphic objects like ["metric", "metric_distr"]
  • the register is now built inside a function, so the loop variables no longer leak into the module namespace
  • new tests for the tag class spec

As discussed in the issue, I left the all_tags filtering change for a separate PR. This overlaps with #1179 in _tags.py, so whichever goes in first, I'll rebase the other.

Would appreciate a review when you have time, especially on the user_facing values I picked.

This branch has not been deployed

No deployments
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.

[ENH] update skpro base framework with sktime upgrades

1 participant