-
Notifications
You must be signed in to change notification settings - Fork 72
Open
Description
The docstrings for class ModelBuilder have multiple issues including but not limited to the following. It is possible that in some cases a solution would involve changing methods' signatures or implementation, not just a docstring.
- Docstrings do not use conventional type names.
E.g.Dictionary,array,Booleanshould be changed todict,ndarray,booletc. - Docstrings do not follow a standard format.
- No need for a new line after
""". - No need for empty lines separating input parameters of methods. No need for double empty lines in any docstring.
- Infinitive without "to" should be used in place of 3rd person form of the verb describing what a function does, e.g. "Initialise model" instead of "Initialises model".
- Extra indentation in Examples is inconsistent.
- The description of a parameter should start on a separate indented line. The first line should be just
extend_idata: bool, default=False(sklearn) orextend_idata: bool, optional(numpy).
- No need for a new line after
- Some information is unnecessary or inaccurate.
Raises: NotImplementedErroris not needed in abstract method. Same withReturns None. - Implementation issues.
raise NotImplementedErrorin abstract methods should be replaced withpassor template implementation (double check this item, as I am not 100% what the best practice is).- The above can be done provided
class ModelBuilder:is replaced withclass ModelBuilder(abc.ABC): get_default_sampler_configshould not takeselfas a parameter.- Consider a concrete
def _save_input_params(): passthat does nothing, but can be overridden. This avoids checkingif hasattr(self, "_save_input_params"):.
- Type annotation issues:
- Types in annotations should match docstrings. This may help fix other issues, e.g. In model_builder, _validate_data changes input type #314 .
- Default
Noneshould always be acceptable, i.e. listed among types. - Declare types of attributes in abstract methods when a concrete implementation is expected to make an assignment. This reduces type check errors and warnings. For example, include line
self.model : pm.Modelinbuild_model()to prevent type check errors atwith self.model:in other methods.
fit()return parameter should be calledidata, notself.- Docstrings should obey reasonable line length limitation, e.g. 100 characters.
- Example for
_generate_and_preprocess_model_data()should setself.coords, as opposed tocoords. Also, it is not clear howself.coordsis used by the ModelBuilder and the model itself. Neither pymc-examples/**/model_builder.html nor test_model_builder.py show this. I personally would be in favour of removing this method, as it is only used insidefit()right beforebuild_model().
Metadata
Metadata
Assignees
Labels
No labels