search

HopkinsIDD / flepiMoP

The Flexible Epidemic Modeling Pipeline
https://flepimop.org
GNU General Public License v3.0
9 stars 4 forks source link

Document, Test, Deprecate/Migrate `gempyor.utils` #246

Open TimothyWillard opened 3 months ago

TimothyWillard commented 3 months ago

The gempyor.utils module could use some work namely documenting the contained objects with a consistent style, creating a comprehensive test suite that ensures expected behavior, and deprecating some functions/classes or moving objects that shouldn't be housed in utils to a more appropriate module.

Documentation: Looks like there is documentation on some functions (ex. rolling_mean_pad or download_file_from_s3), but most do not nor does the module itself. Also need to make documentation consistent, looks like the existing documentation is some kind of variant on the Google style guide so I propose sticking to that. A part of documentation is also expanding the usage of type hints that provide users with documentation on expected inputs.

Tests: Looks like there are some existing tests in tests/utils/test_file_paths.py, tests/utils/test_utils.py, and tests/utils/test_utils2.py. It would be nice to create a consistent style across these tests and expand them to ensure that expected behavior and usage are covered. I also would suggest placing unit tests in a more structured format, namely:

  1. Use a test fixture to test only one set of behavior at a time.
  2. Group test fixtures related to the same object using a class.
  3. Create individual files for most objects.

Deprecate/Migrate: This is a longer term goal, but these functions should be moved to a more descriptive home. The name utils gives me very little info about the contents contained but names like io, math, etc. are informative. Below are some brief thoughts on each function:

Bit of a lengthy issue so I don't expect to complete it overnight and I think putting some code/documentation/tests into an initial PR to highlight the value would be good (will do soon). But any initial thoughts @jcblemai or @emprzy?

emprzy commented 3 months ago

I'll write some documentation! Thanks for looping me in.

TimothyWillard commented 3 months ago

List of functions/classes to document/test: List of functions/classes to document:

emprzy commented 3 months ago

I'll start working at the top half!

emprzy commented 3 months ago

Sorry I lied....I think I'll just go ahead and do all of them @TimothyWillard. I don't have much else to do this afternoon

emprzy commented 3 months ago

A couple questions for @jcblemai before I push changes.

jcblemai commented 3 months ago

Tagging @fang19911030 because he wrote these functions :)

These are string arguments :)

fang19911030 commented 3 months ago

Hi @emprzy , as @jcblemai said, these are string arguments, do you ask the meaning of these two arguments in the context of model execution?

jcblemai commented 3 months ago

in the function 'create_resume_out_filename' and 'create_resume_input', what is the arg 'liketype' for?

liketype takes two value: chimeric or global, depending on which "chain" of the dual chain mcmc scheme we are using (the one for all subpopulation vs the one that treats them as independent)

in the function 'create_resume_file_names_map', what are the args 'reume_run_index', and 'flepi_prefix' for?

These are part of the filenames, there are used to build the filenames of the file to download from resume.

emprzy commented 3 months ago

@fang19911030 yes, I mean in the context of model execution. just not sure what 'liketype' is referencing and want to be able to properly document it

jcblemai commented 3 months ago

Sorry I had formatting issue in my last message, let me know if you need more info about liketype

TimothyWillard commented 3 weeks ago

@emprzy Do you have plans to follow up GH-260 with a PR for adding/restructuring the unit tests for the functions documented?

emprzy commented 3 weeks ago

@emprzy Do you have plans to follow up GH-260 with a PR for adding/restructuring the unit tests for the functions documented?

I didn't have that on my to do list, but if you want me to do that I could? Why do you ask?

TimothyWillard commented 3 weeks ago

No, if you have other items on your todo list that's fine, I'm just following up to understand what is left to do here. List of functions/classes to be tested/have tests restructured: