Skip to content

Multiple issues with ModelBuilder docstrings #608

@avm19

Description

@avm19

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.

  1. Docstrings do not use conventional type names.
    E.g. Dictionary, array, Boolean should be changed to dict, ndarray, bool etc.
  2. Docstrings do not follow a standard format.
    1. No need for a new line after """.
    2. No need for empty lines separating input parameters of methods. No need for double empty lines in any docstring.
    3. 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".
    4. Extra indentation in Examples is inconsistent.
    5. The description of a parameter should start on a separate indented line. The first line should be just extend_idata: bool, default=False (sklearn) or extend_idata: bool, optional (numpy).
  3. Some information is unnecessary or inaccurate. Raises: NotImplementedError is not needed in abstract method. Same with Returns None.
  4. Implementation issues.
    1. raise NotImplementedError in abstract methods should be replaced with pass or template implementation (double check this item, as I am not 100% what the best practice is).
    2. The above can be done provided class ModelBuilder: is replaced with class ModelBuilder(abc.ABC):
    3. get_default_sampler_config should not take self as a parameter.
    4. Consider a concrete def _save_input_params(): pass that does nothing, but can be overridden. This avoids checking if hasattr(self, "_save_input_params"):.
  5. Type annotation issues:
    1. Types in annotations should match docstrings. This may help fix other issues, e.g. In model_builder, _validate_data changes input type  #314 .
    2. Default None should always be acceptable, i.e. listed among types.
    3. 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.Model in build_model() to prevent type check errors at with self.model: in other methods.
  6. fit() return parameter should be called idata, not self.
  7. Docstrings should obey reasonable line length limitation, e.g. 100 characters.
  8. Example for _generate_and_preprocess_model_data() should set self.coords, as opposed to coords. Also, it is not clear how self.coords is 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 inside fit() right before build_model().

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions