`gempyor` API docstring refurbishment #468

emprzy · 2025-01-15T20:19:25Z

Describe your changes.

This pull request:

Adds inline docstring documentation to important (as identified by flepiMoP users) submodules, classes, methods, etc. within gempyor
Improves existing docstring documentation, when reasonable to do so
Removes some poor documentation, so that when we use Sphinx in the future as a tool for documenting gempyor, it does not propagate poor documentation

As I see it, the gempyor API mainly consists of:

ModelInfo class from model_info.py
Parameters class from parameters.py, and some associated methods
GempyorInference class from inference.py, and some associated methods
Compartments class from compartments.py, and some associated methods
and two functions from utils.py (read_df and write_df)

Note, before narrowing my focus on guidance from Carl, I also wrote some docstring in two files (calibrate.py and cli.py). I do not think of these as being integral parts of the gempyor API, but because I worked on them in in the same sweep of work that I worked on the others, they are also included in this PR with some docstring improvements.

What does your pull request address? Tag relevant issues.

This pull request addresses GH #462.

…mprzy/flepiMoP into gempyor-docstring-improvements

TimothyWillard

Overall looks good, I think an improvement over the current state. A few themes:

What's up with putting the file name as the header of a module docstrings? Seems redundant.
There are some typing signatures that are off, I'd rather have no type signature than an incorrect one so if it's not clear let's leave it for another PR.
There were some docstrings of CLI commands that were edited in ways that I think will look off when using --help, I think we need to be careful about not applying the same docstring style to click commands.

flepimop/gempyor_pkg/src/gempyor/calibrate.py

flepimop/gempyor_pkg/src/gempyor/cli.py

flepimop/gempyor_pkg/src/gempyor/inference.py

flepimop/gempyor_pkg/src/gempyor/model_info.py

flepimop/gempyor_pkg/src/gempyor/parameters.py

emprzy · 2025-01-24T15:21:38Z

This PR can also be used to address GH #431 .

…mprzy/flepiMoP into gempyor-docstring-improvements

pearsonca

oops, seems like this wasn't submitted

flepimop/gempyor_pkg/src/gempyor/calibrate.py

flepimop/gempyor_pkg/src/gempyor/compartments.py

pearsonca · 2025-01-27T19:18:23Z

flepimop/gempyor_pkg/src/gempyor/inference.py

+    Attributes:
+        seir_modifiers_scenario:
+        outcome_modifiers_scenario:
+        modinf: A `ModelInfo` object.
+        already_built: Flag to determine if necessary objects have been built.
+        autowrite_seir: Flag to automatically write SEIR data after simulation.
+        static_sim_arguments: Dictionary containing static simulation arguments.
+        do_inference: Flag to determine if model should be run with inference.
+        silent: Flag indicating whether to supress output messaging.
+        save: Flag indicating whether simulation results should be saved.


looking at the sphinx autdoc syntax, seems like these can defined inline with the various self.attributeA = ... declarations in init

@TimothyWillard feels to me like we should prefer that approach, what do you think?

I kinda see these primarily as internal structure that we shouldn't advertise via the API anyway.

Assuming you're referring to this syntax: https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#doc-comments-and-docstrings?

I haven't used it before (only used the Attributes section docstring), but it looks easier so sounds good to me. Can also do this with the sphinx specific PR when we get there.

I kinda see these primarily as internal structure that we shouldn't advertise via the API anyway.

Agree, but these are advertised as public by naming convention and it is beyond the scope of this PR to change that. We should create a follow up to address separately and treat as public for now.

@TimothyWillard , so is it your opinion that we leave the attributes in this documentation, or should I remove them since they aren't part of the public-facing API?

They are a part of the public facing API right now, so I think we should document it and if we want to change that we should do that in a follow up.

Sounds good. In that case I have responded to all requested changes and this can either be re-reviewed or merged.

pearsonca

Looks fine overall - probably fix the one case of repeating the information from the type hint in the documentation, which sphinx should make unnecessary

flepimop/gempyor_pkg/src/gempyor/parameters.py

TimothyWillard

Few minor things, all but one are suggestions that can be taken or left. Only thing I couldn't suggest in the PR review GUI was the import order thing (I think because it involves LOC that weren't edited). Otherwise LGTM let's push forward for sphinx!

TimothyWillard · 2025-01-31T16:41:16Z

flepimop/gempyor_pkg/src/gempyor/compartments.py

        """
        This method is called by the constructor if the compartments are not loaded from a file.
        It will parse the compartments and transitions from the configuration files.
-        It will populate self.compartments and self.transitions.
+        It will populate dynamic class attributes `self.compartments` and `self.transitions`.


Suggested change

It will populate dynamic class attributes `self.compartments` and `self.transitions`.

It will populate dynamic class attributes `compartments` and `transitions`.

TimothyWillard · 2025-01-31T16:41:55Z

flepimop/gempyor_pkg/src/gempyor/calibrate.py

+Facilitates the calibration of the model using the `emcee` MCMC sampler. 
+Provides CLI options for users to specify simulation paramters. 


Suggested change

Facilitates the calibration of the model using the `emcee` MCMC sampler.

Provides CLI options for users to specify simulation paramters.

Facilitates the calibration of the model using the `emcee` MCMC sampler.

Provides CLI options for users to specify simulation parameters.

Split into header and description (assuming that's the intent?) and typo.

TimothyWillard · 2025-01-31T16:43:14Z

flepimop/gempyor_pkg/src/gempyor/compartments.py

+    def parse_parameters(
+        self, parameters: np.ndarray, parameter_names: list, unique_strings: list
+    ) -> np.ndarray:


Sorry, realizing I did not reply, not complete is better, we can go back and complete the picture later.

TimothyWillard · 2025-01-31T16:44:06Z

flepimop/gempyor_pkg/src/gempyor/compartments.py

-    def get_compartments_explicitDF(self):
+    def get_compartments_explicitDF(self) -> pd.DataFrame:
+        """
+        Returns a copy of the compartments information DataFrame; all columns receive a 'mc_' prefix.


Suggested change

Returns a copy of the compartments information DataFrame; all columns receive a 'mc_' prefix.

Returns a copy of the compartments information DataFrame.

All columns receive a 'mc_' prefix.

TimothyWillard · 2025-01-31T16:47:25Z

flepimop/gempyor_pkg/src/gempyor/inference.py

@@ -24,6 +45,7 @@
 from . import seir, model_info
 from . import outcomes, file_paths
 from .utils import config, Timer, read_df, as_list
+from typing import Literal


Up to you if you want to do this now or later, but let's try to follow PEP8 for imports going forward (i.e. move this up to stdlib imports rather than with local pkg imports): https://peps.python.org/pep-0008/#imports

TimothyWillard · 2025-01-31T16:48:41Z

flepimop/gempyor_pkg/src/gempyor/model_info.py

+"""
+Defines the `ModelInfo` class (and associated methods), used for setting up and 


Suggested change

"""

Defines the `ModelInfo` class (and associated methods), used for setting up and

"""

Abstractions for interacting with models as a monolithic object.

Defines the `ModelInfo` class (and associated methods), used for setting up and

Add header line.

Co-authored-by: Carl A. B. Pearson <[email protected]>

Docstring updates (compartments.py is finished)

287eb04

emprzy changed the base branch from main to dev January 15, 2025 20:19

emprzy and others added 5 commits January 22, 2025 14:26

Updating docstring in inference.py, model_info.py, parameters.py

3ee7514

Linting with black

572f8f7

Merge branch 'dev' into gempyor-docstring-improvements

37665da

Documentation update in model_info

b680cb8

Merge branch 'gempyor-docstring-improvements' of https://github.com/e…

3981fab

…mprzy/flepiMoP into gempyor-docstring-improvements

emprzy marked this pull request as ready for review January 22, 2025 20:01

TimothyWillard mentioned this pull request Jan 23, 2025

Expand Scalar Likelihoods Manually In Statistics.llik #478

Merged

TimothyWillard linked an issue Jan 23, 2025 that may be closed by this pull request

[Feature request]: Ensure docstring documentation exists in major sub-modules of gempyor #462

Open

TimothyWillard added gempyor Concerns the Python core. high priority High priority. docstring Relating to in-code documentation. next release Marks a PR as a target to include in the next release. quick issue Short or easy fix. labels Jan 23, 2025

TimothyWillard added this to the Software Quality milestone Jan 23, 2025

Changing a data type hint in compartments.py

ab85156

emprzy requested review from pearsonca and TimothyWillard January 24, 2025 14:31

TimothyWillard requested changes Jan 24, 2025

View reviewed changes

emprzy added 3 commits January 24, 2025 14:26

Implementing suggestions from @TimothyWillard review

2b777c5

Merge branch 'gempyor-docstring-improvements' of https://github.com/e…

759f8a1

…mprzy/flepiMoP into gempyor-docstring-improvements

Updating weird formatting on a CLI command

5ad4b06

pearsonca reviewed Jan 30, 2025

View reviewed changes

emprzy requested review from TimothyWillard and pearsonca January 31, 2025 15:28

Update calibrate.py

2301fe1

pearsonca approved these changes Jan 31, 2025

View reviewed changes

flepimop/gempyor_pkg/src/gempyor/parameters.py Outdated Show resolved Hide resolved

TimothyWillard approved these changes Jan 31, 2025

View reviewed changes

Update flepimop/gempyor_pkg/src/gempyor/parameters.py

2a25890

Co-authored-by: Carl A. B. Pearson <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`gempyor` API docstring refurbishment #468

`gempyor` API docstring refurbishment #468

emprzy commented Jan 15, 2025 •

edited

Loading

TimothyWillard left a comment

emprzy commented Jan 24, 2025

pearsonca left a comment

pearsonca Jan 27, 2025

TimothyWillard Jan 31, 2025 •

edited

Loading

emprzy Jan 31, 2025

TimothyWillard Jan 31, 2025 •

edited

Loading

emprzy Jan 31, 2025

pearsonca left a comment

TimothyWillard left a comment

TimothyWillard Jan 31, 2025

TimothyWillard Jan 31, 2025

TimothyWillard Jan 31, 2025

TimothyWillard Jan 31, 2025

TimothyWillard Jan 31, 2025

TimothyWillard Jan 31, 2025

	It will populate dynamic class attributes `self.compartments` and `self.transitions`.
	It will populate dynamic class attributes `compartments` and `transitions`.

		Facilitates the calibration of the model using the `emcee` MCMC sampler.
		Provides CLI options for users to specify simulation paramters.

-        Returns a copy of the compartments information DataFrame; all columns receive a 'mc_' prefix.
+        Returns a copy of the compartments information DataFrame.
+        All columns receive a 'mc_' prefix.

		"""
		Defines the `ModelInfo` class (and associated methods), used for setting up and

gempyor API docstring refurbishment #468

Are you sure you want to change the base?

gempyor API docstring refurbishment #468

Conversation

emprzy commented Jan 15, 2025 • edited Loading

Describe your changes.

What does your pull request address? Tag relevant issues.

TimothyWillard left a comment

Choose a reason for hiding this comment

emprzy commented Jan 24, 2025

pearsonca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyWillard Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyWillard Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearsonca left a comment

Choose a reason for hiding this comment

TimothyWillard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`gempyor` API docstring refurbishment #468

`gempyor` API docstring refurbishment #468

emprzy commented Jan 15, 2025 •

edited

Loading

TimothyWillard Jan 31, 2025 •

edited

Loading

TimothyWillard Jan 31, 2025 •

edited

Loading