Behaviour of `get_tag` is different from standard expectation from `get` or `getattr`

[yarnabrina]

get_tag is behaving differently from get or getattr in python. Usually they return default value provided if key is missing. In skbase, it fails even when default value is provided, unless one specifically pass an additional argument to control error behaviour.

This is not really a bug, but a behaviour that is probably not very intuitive to python users.

>>>
>>> from skbase.base import BaseObject
>>>
>>> BaseObject().get_tag("abcd")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/anirban/conda-environments/sktime/lib/python3.10/site-packages/skbase/base/_base.py", line 578, in get_tag
    return self._get_flag(
  File "/home/anirban/conda-environments/sktime/lib/python3.10/site-packages/skbase/base/_tagmanager.py", line 147, in _get_flag
    raise ValueError(f"Tag with name {flag_name} could not be found.")
ValueError: Tag with name abcd could not be found.
>>>
>>> BaseObject().get_tag("abcd", tag_value_default="pqrs")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/anirban/conda-environments/sktime/lib/python3.10/site-packages/skbase/base/_base.py", line 578, in get_tag
    return self._get_flag(
  File "/home/anirban/conda-environments/sktime/lib/python3.10/site-packages/skbase/base/_tagmanager.py", line 147, in _get_flag
    raise ValueError(f"Tag with name {flag_name} could not be found.")
ValueError: Tag with name abcd could not be found.
>>>
>>> ARIMA().get_tag("abcd", tag_value_default="pqrs", raise_error=False)
'pqrs'
>>>

[fkiraly]

fully agree - it's in many places though, and if we change it we do not notice the places that rely on raising an error, as we switch off error conditions without switching them one.

Should we change with deprecation?

[fkiraly]

(in support of your argument, the behaviour is also inconsistent with get_class_tag)

[Deep-Axe]

Fix for this has been implemented in #555

The change uses a sentinel to distinguish "caller passed no default" from "caller passed a value (including None)", mirroring the semantics of getattr().

After the fix:

  BaseObject().get_tag("abcd")                          # raises ValueError
  BaseObject().get_tag("abcd", tag_value_default="pqrs") # returns 'pqrs'
  BaseObject().get_tag("abcd", None)                    # returns None instead of raising
  BaseObject.get_class_tag("abcd", "pqrs")              # unchanged 

@fkiraly, the PR focuses on the case where an explicit default was being ignored. Regarding the question of whether get_tag("abcd") (with no default provided) should eventually return None instead of raising: I'm happy to open a follow-up PR to start a FutureWarning deprecation cycle that changes the raise_error default from True to False.

[fkiraly]

Hm, having thought about this: I think we should not change the default behaviour of get_tag.

The reason for this is that:

• per design expectation, default tag values should be set in a common super class, and not in the get_tag method.
• more generally, default values for tag should be managed globally (inside a package) and not locally (at call)
• in most downstream uses which do follow this design, it feels preferable to have a failure if the tag is not actually present, but requested

If we change the behaviour along the lines of dict.get, then the above would no longer be maintained - instead, the management of value defaults would move towards the local call.

This raises the question, due to the inconsistency with get_class_tag that @Deep-Axe has pointed out - should then not get_class_tag default behaviour change?