-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue273 default values for configuration settings #324
Issue273 default values for configuration settings #324
Conversation
Codecov Report
@@ Coverage Diff @@
## master #324 +/- ##
==========================================
- Coverage 99.57% 99.55% -0.02%
==========================================
Files 56 56
Lines 3040 3149 +109
==========================================
+ Hits 3027 3135 +108
- Misses 13 14 +1
Continue to review full report at Codecov.
|
This pull request introduces 1 alert when merging 4ef0b5a into 2fb6108 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging f8825ed into 2fb6108 - view on LGTM.com new alerts:
|
I like the way backend classes (including mixins like ChunkingBackend) declare their own default parameters, but the method for looking them up by accessing the class object seems a bit overcomplicated. I'd prefer to use standard inheritance for this. How about letting backend classes declare a property that, when accessed, returns the list of default parameter values. The implementation of the property method can then decide whether the defaults are inherited from superclasses or not. The simple case (inherited from AnnifBackend) would just return DEFAULT_PARAMS. Something like: class AnnifBackend:
DEFAULT_PARAMS = {'limit': 100}
@property
def default_params(self):
return self.DEFAULT_PARAMS
class ChunkingBackend:
DEFAULT_PARAMS = {'chunksize': 2}
class FastTextBackend(ChunkingBackend, AnnifBackend):
DEFAULT_PARAMS = {'loss', 'hs'} # and others
@property
def default_params(self):
# Start with AnnifBackend defaults
params = AnnifBackend.default_params(self).copy()
# override with ChunkingBackend defaults
params.update(ChunkingBackend.default_params(self))
# override with our own defaults
params.update(self.DEFAULT_PARAMS)
return params Okay, maybe this isn't the simplest method either, but at least backends have full control over which default parameters they wish to inherit from upper classes. Note that FastTextBackend already has a |
On second thought, maybe |
This pull request introduces 1 alert when merging bf58247 into 6ac8431 - view on LGTM.com new alerts:
|
annif/backend/backend.py
Outdated
def default_params(self): | ||
return self.DEFAULT_PARAMS | ||
|
||
def fill_params_with_defaults(self, params): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the params
field actually be better to be implemented as a property, and then this would be decorated with @params.setter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of params
being a property. It could be implemented in such a way that it transparently fills in default parameters before returning a dict. And it could have a setter used to set manual parameters (which would be stored in an internal field, say _params
). Actually, the filling in could also be done within the setter - then it would only happen once instead of on every access. Accessing the params
property would then just return self._params
or perhaps a copy()
to be safe.
This pull request introduces 1 alert when merging 420d3d1 into e449223 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 91136a6 into e449223 - view on LGTM.com new alerts:
|
According to this thread the alert could be ignored. |
Regarding the LGTM alert
@property
def params(self):
params = {}
params.update(self.default_params())
params.update(self.config_params)
return params With the above implementation of The benefit of starting from an empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to merge
Edited Wiki pages to shortly mention the parameter defaulting:
|
Implemented one initial approach for discussion.
In this good is that default parameters can be included only in the relevant base classes for a backend (e.g.
chunksize
inChunkingBackend
). The Fasttext and VW params could be included toDEFAULT_PARAMS
too (with a bit of work for passing only the backend specific params to the backends).Not-so-good is maybe the location of this, should it be in
project.py
instead?Another completely different approach could be to use to use ConfigParser's defaults implementation.