[ENH] object config interface#140
Conversation
for more information, see https://pre-commit.ci
… object-config
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| assert not fixture_class_parent_instance._has_implementation_of("some_method") | ||
|
|
||
|
|
||
| class ConfigTester(BaseObject): |
Check warning
Code scanning / CodeQL
`__eq__` not overridden when adding attributes
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 82.64% 82.68% +0.04%
==========================================
Files 32 32
Lines 2316 2322 +6
==========================================
+ Hits 1914 1920 +6
Misses 402 402
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
RNKuhns
left a comment
RNKuhns
left a comment
There was a problem hiding this comment.
@fkiraly I'm fine with the logic and implementations. But can you go through the docstrings to make sure they follow the numpydoc API.
Otherwise they generate lots of formatting errors/warnings when rendered by sphinx (makes it tough to understand success or failure of actual doc buildingg/formatting)
|
|
||
| Parameters | ||
| ---------- | ||
| flag_attr_name : str, optional, default = "_flags" |
There was a problem hiding this comment.
Minor tweak, but numpydoc notes that if the default value is used then you don't put "optional" part of parameter docstring (e.g., you'd just put flag_attr_name : str, default="_flags").
They also show no spaces around equal sign.
Can you take a quick look at the docstrings in the PR and update these.
There was a problem hiding this comment.
I see, you would use "optional" only if you have sth like None. Fixed in all parallel cases
|
|
||
| Returns | ||
| ------- | ||
| collected_flags : dict |
There was a problem hiding this comment.
Per numpydoc if there is only a single return, just the return type can be listed on first line of "returns" description.
There was a problem hiding this comment.
Can you take a look at the returns section of all the docstrings in this PR for conformity with this.
| Parameters | ||
| ---------- | ||
| flag_name : str | ||
| Name of flag to be retrieved |
There was a problem hiding this comment.
The description lines for parameters should end with periods (e.g., Name of flag to be retrieved).
Can you take a look through docstrings in the PR and make sure the periods are there?
There was a problem hiding this comment.
ok, fixed in all instances.
|
|
||
| Raises | ||
| ------ | ||
| ValueError if raise_error is True i.e. if flag_name is not in self.get_flags( |
There was a problem hiding this comment.
Move the description of when the ValueError is raised to the line below the error type (and indent like other descriptions).
|
|
||
| Returns | ||
| ------- | ||
| Self : Reference to self. |
There was a problem hiding this comment.
Move return type description to line below return type (and remove colon).
|
|
||
| Returns | ||
| ------- | ||
| Self : |
| return self | ||
|
|
||
| update_dict = {key: tags_est[key] for key in tag_names} | ||
| def get_config(self): |
There was a problem hiding this comment.
Not a blocking change, but just noting that later we'll want to generalize get_config to get global config and local config (currently only handles local config).
There was a problem hiding this comment.
hm, I thought the future logic was going to be, local config overrides global config, and the return is global.update(local)
There was a problem hiding this comment.
@fkiraly that's actually what I meant. We'll need an extension point so that we users can specify global config locations that implement something that aligns with the protocol (e.g, can point to their own packages global get_config). The idea would be to get skbase global configs, user global configs and then local configs.
Analagous to walking the MRO and collecting tags, but walking specified global configs.
|
I think I've addressed all the above, including parallel locations. Btw, if we lint |
@fkiraly I've opened the issue. And it looks like our thoughts on this are timely. There is a new PR in |
4 participants
port of sktime/sktime#3822 to
skbaseThis PR adds a config management capabilities to all
sktimeobjects, similar tosklearn's new config interface, via two methodsget_configandset_config.It is based on the flag manager mixin introduced in sktime/sktime#3630 (ported to
skbasein #138)A STEP is available here: sktime/enhancement-proposals#29
Includes:
get_configandset_configinterface