Skip to content

Suggestions for PR #891 - BaseObject with dynamic tag handling functionality and dedicated get_tag interface point#1099

Closed
fkiraly wants to merge 10 commits intomainfrom
891-suggestions
Closed

Suggestions for PR #891 - BaseObject with dynamic tag handling functionality and dedicated get_tag interface point#1099
fkiraly wants to merge 10 commits intomainfrom
891-suggestions

Conversation

@fkiraly
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly commented Jul 3, 2021

Suggested additions for PR #891. Can't directly PR into the 891 PR apparently since it's coming from a fork (can I?), but it's meant as a PR to that.

This adds dynamic tag functionality to the BaseObject class.
Methods added: getters get_tags, get_tag and setters set_tags and mirror_tags. These make use of dynamic tags that can be set in construction of any and override class tags and their inheritance (read by _all_tags). The methods get_tags and get_tag are meant to be the primary interface point for tags, replacing current _all_tags or external has_tag (from testing module).

This should be fully downwards compatible, as the old interface points (all_tags, has_tag) have not been removed.

Besides, I've cleaned up the merge conflicts from main and added docstrings at the module level.

To add more detail on how the tag related methods work:

post-PR, tags would work like this:

  • each class will have static tags, that's the class variable _tags: dict. Individual static tags are considered to override each other as per normal nested inheritance. E.g., if a parent class and the class have different values for a static tag, the younger class' tag value counts.
  • each object can, in addition, have dynamic tags, in the object variable _tags_dynamic: dict. These are expected to be initialized in the constructor, for instance in cases where behaviour described by tags depends on components or hyper-parameters. Dynamic tags are always considered to override the static tags - e.g., if the object's class or a parent class have a static tag of the same name, the dynamic tag's value overrides any static tag's value.

The new methods make use of this inheritance logic:

  • get_tags() collects all static and dynamic tags and their values and returns them as a dict (keys = tag names, values = tag values). It overrides values precisely as described above, in the return dictionary.
  • get_tag(tag_name, tag_value_default=None) retrieves the value of an individual tag with name tag_name, and optionally allows to specify a default value if the tag is not found. The inheritance/override logic is the same as in get_tags.
  • set_tags(dict) sets dynamic tags according to keys (=tag names) and values (= tag values) in the argument dictionary. It is expected to be used in constructors typically.
  • mirror_tags(estimator, tag_set=None) is a shorthand version of set_tags, where it sets tags according to their get_tag values in estimator. The tags that are mirrored/copied in this way can be restricted by specifying a list of tag names in tag_set, otherwise all tags are copied over. The mirroring is by-reference and not by-value (if the type allows this), but tags are not expected to be mutated.

@fkiraly fkiraly requested a review from mloning as a code owner July 3, 2021 12:56
@fkiraly fkiraly requested review from RNKuhns and aiwalter July 3, 2021 12:56
@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 3, 2021

The dynamic tags would be very useful in resolving some inconsistencies in cases where behaviour (and thus the "true" tag) depends on components or parameter settings, e.g., determining the requires-fh-in-fit tag of composite forecasters based on the component forecasters. See, e.g., #1091.

@fkiraly fkiraly changed the title Suggestions for PR #891 Suggestions for PR #891 - BaseObject with dynamic tag handling functionality and dedicated get_tag interface point Jul 3, 2021
@mloning
Copy link
Copy Markdown
Contributor

mloning commented Jul 5, 2021

Could you explain what the different methods do (here or in a call)? Does this allow to have both static and dynamic tags at the same time?

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 5, 2021

Could you explain what the different methods do (here or in a call)? Does this allow to have both static and dynamic tags at the same time?

Yes, and yes - I'll update this PR's description, and can write a STEP if you think it's substantial enough a change.

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 5, 2021

updated, see above

@mloning
Copy link
Copy Markdown
Contributor

mloning commented Jul 6, 2021

Okay, thanks, let's discuss in the next technical call! I'm generally in favour of adding dynamic tags as proposed in this PR.

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 6, 2021

alright - as discussed, not in the next release though

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 13, 2021

picking up from #981 now

fkiraly added a commit that referenced this pull request Jul 17, 2021
Rework of PR #1099 which is in turn a rework of PR #891 (by @RNKuhns), based on discussion recorded in #981 on tags.

Adds a `BaseObject` class which adds dynamic tag functionality. `BaseObject` contains `BaseEstimator` functionality sans `fit` state change related interfaces.
The new functionality replaces the inconsistent mixture of old interface points (`all_tags`, `has_tag`) in all downstream dependencies.

post-PR, tags work like this:
* each class will have "class tags", stored in the content of the class variable `_tags: dict`. Individual class tags are considered to override each other as per normal nested inheritance. E.g., if a parent class and the class have different values for a class tag, the younger class' class tag value counts.
* each object can, in addition, have dynamic tags, in the object variable `_tags_dynamic: dict`. These are expected to be initialized in the constructor, for instance in cases where behaviour described by tags depends on components or hyper-parameters. Dynamic tags are always considered to override the class tags - e.g., if the object's class or a parent class have a static tag of the same name, the dynamic tag's value overrides any class tag's value.

The new tag related methods make use of this inheritance logic are as follows.

class methods:
* `get_class_tags()` collects all class tags and their values and returns them as a `dict` (keys = tag names, values = tag values). It overrides values precisely as described above, in the return dictionary. This replaces the old `_all_tags`.
* `get_class_tag(tag_name, tag_value_default=None)` retrieves the value of an individual tag with name `tag_name`, and optionally allows to specify a default value if the tag is not found. The inheritance/override logic is the same as in `get_class_tags`.

object methods:
* `get_tags()` collects all class tags and dynamic tags and their values and returns them as a `dict` (keys = tag names, values = tag values). It overrides values precisely as described above, in the return dictionary.
* `get_tag(tag_name, tag_value_default=None)` retrieves the value of an individual tag with name `tag_name`, and optionally allows to specify a default value if the tag is not found. The inheritance/override logic is the same as in `get_tags`.
* `set_tags(**tag_dict)` sets dynamic tags according to keys (=tag names) and values (= tag values) in the argument dictionary. It is expected to be used in constructors typically.
* `mirror_tags(estimator, tag_set=None)` is a shorthand version of `set_tags`, where it sets tags according to their `get_tag` values in `estimator`. The tags that are mirrored/copied in this way can be restricted by specifying a list of tag names in `tag_set`, otherwise all tags are copied over. The mirroring is by-reference and not by-value (if the type allows this), but tags are not expected to be mutated.
@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 17, 2021

superseded by the now merged #1134

@fkiraly fkiraly closed this Jul 17, 2021
@fkiraly fkiraly deleted the 891-suggestions branch August 27, 2021 13:20
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.

3 participants