Repository navigation
Conversation
Adds _BaseTag, the register assembly and check_tag_is_valid, with three tags covering the three tag_type shapes. Excludes _registry from the all_objects crawl so tag classes are not returned as objects.
Completes the register at 22 tags and adds two package-wide tests: every tag declared anywhere must be registered, and every declared value must satisfy its declared tag_type.
The all_objects docstring has pointed at a registry.all_tags utility that did not exist; this adds it.
A misspelt tag name used to return an empty list with no indication of what went wrong.
Also corrects three Effect sections: the metric test class is TestAllPtMetrics, and info:pred_type and info:y_type are read by EstimatorFixtureGenerator to choose which losses a model is tested against, not to choose test data.
The same tag descriptions were copied across two READMEs, two templates and the package layer page. The register is now the single authority.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2451 +/- ##
=======================================
Coverage ? 87.78%
=======================================
Files ? 220
Lines ? 11915
Branches ? 0
=======================================
Hits ? 10459
Misses ? 1456
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
phoeenniixx
left a comment
phoeenniixx
left a comment
There was a problem hiding this comment.
I think we should not remove the comments and change the readme for the extension templates. There is no harm in having the explaination in two places.
This registry is mainly to access the info via code, but some contributors just look at the extension templates
Also do we want all these tests for the tags? I thought this registry is more of a documentaion work, so there can be almost no possibility of getting any bug there?
I think some minimal tests should be enough. See how sktime does it.
I have not looked very deeply into those tests, but i feel there are a lot of tests here that can be simplified or removed (like checking if a specific param is present in the doc).
Or do you think all these tests are necessary?
| # todo: update all tag values to match your model | ||
| # | ||
| # Human-readable model name — MUST match the model class name. | ||
| # Valid values: str |
There was a problem hiding this comment.
maybe we should leave these comments as well. This makes things easier ig
2 participants
Reference Issues/PRs
Fixes #2333. Follows up on the review of #2334.
What does this implement/fix? Explain your changes.
Adds a class-based tag register to
pytorch_forecasting/_registry, following the patternsktimeandskprouse, and makes it the single place tag semantics are documented._registry/_tags.pycheck_tag_is_valid(tag_name, tag_value)all_tags(parent_types, return_names, as_dataframe)all_objectsrejects unknownfilter_tagsdocs/source/registry.rst, linked from the v1 and v2 API toctreesEach tag documents its possible values, its default, and what reads it, as asked for in the review of #2334. The same descriptions were previously copied across five places (both template READMEs, both template
_model_pkg.pycomment blocks, the package layer page); all five now point at the register.What should a reviewer concentrate their feedback on?
parent_typeforinfo:pred_typeandinfo:y_type.Did you add any tests for the change?
Yes,
_registry/tests/test_tags.py, 68 cases. Three are worth calling out:test_every_tag_is_documentedtest_every_declared_tag_is_registeredandtest_every_declared_tag_value_is_validtest_metric_type_enum_matches_test_all_metricsandtest_object_type_enum_covers_test_class_registryAny other comments?
Reading each tag's consumers turned up three things
capability:*family has no defaults, so an omitted tag reads back asNone, notFalse.info:pred_typeis missing from every v2 package andinfo:y_typefrom three, although the v2 template asks for them. The fallback to[]is silent and narrows which losses those models are tested against.TFTForecasterin [ENH] Addforecastersupport and remove_pkgclass for new API #2434 has the same gap.info:metric_name,capability:quantile_generationandshape:adds_quantile_dimensionhave no consumer anywhere, including docs and tests.PR checklist