HopkinsIDD / flepiMoP

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

`utils.py` function `profile()` has potentially unused arguments `sort_by`, `lines_to_print`, and `strip_dirs` #306

Open emprzy opened 2 months ago

emprzy commented 2 months ago

Label

enhancement, good first issue

Priority Label

low priority

Is your feature request related to a problem? Please describe.

While documenting the gempyor.utils module I noticed that several of the parameters in the profile() function are unused in the body of the function. If these arguments are truly not vital to the functions of the function, then we should probably remove...but need to make sure that they are not used elsewhere in a call somewhere.

Is your feature request related to a new application, scenario round, pathogen? Please describe.

No response

Describe the solution you'd like

Code base needs to be checked to see if the arguments in question are used in a call somewhere to verify whether or not tgey can be removed.

emprzy commented 2 months ago

Okay actually after looking into this a little bit more I think sort_by, lines_to_print, and strip_dirs are included for compatibility with some profiling analysis tools and custom handling (if such is necessary). So even though they don't get called within the body of the function definition they are still useful to keep. I think I will close the issue!

pearsonca commented 2 months ago

@emprzy does it potentially make to do something like: https://stackoverflow.com/questions/26515595/how-does-one-ignore-unexpected-keyword-arguments-passed-to-a-function

That could address the signature issue, while also making it clear those arguments have no use in our method (this is akin to an ellipsis in R function signature or defining C++ functions with types but no variable names).

emprzy commented 2 months ago

@pearsonca Sure, that looks like a viable solution. I'll look into implementing that on my next utils.py commit.

emprzy commented 1 month ago

@pearsonca , Hi, circling back on this. I wanted to ask something re: your suggestion about handling unused arguments. I've gone through and used flake8-unused-arguments to get an idea of where the "unused" (I say in quotes only because they may be used elsewhere) arguments are within gempyor. Is your suggestion that we delete the explicit references to these parameters in function definitions and just include a **kwargs parameter in their place? Just want to make sure I'm correctly understanding the translation from the stack overflow link.

pearsonca commented 1 month ago

Is your suggestion that we delete the explicit references to these parameters in function definitions and just include a **kwargs parameter in their place?

Yes.

I think there's one minor caveat: when those hypothetical external tools attempt to use the arguments we don't actually support, we might want the code to emit something indicating those args aren't supported. That could be as simple as collecting them in kwargs, checking for non-empty status and then warning a note about which arguments + unsupported.

Clear?

emprzy commented 1 month ago

I believe so. Removing "unused" arguments, including a **kwargs argument, and including something to check if there are any kwargs, then throwing a warning if so. Do you want that catch/throw to happen within the function? @pearsonca

TimothyWillard commented 1 month ago

Alternatively, is this a function that we can just delete? Doesn't look like it is in active use anywhere:

(venv) twillard@epid-iss-mbp ~/D/G/H/flepiMoP (GH-191/longleaf-batch-submission)> grep -rn flepimop/gempyor_pkg/src/gempyor/ --exclude-dir=__pycache__ -e "profile"
flepimop/gempyor_pkg/src/gempyor//simulate.py:165:from gempyor.utils import config, as_list, profile
flepimop/gempyor_pkg/src/gempyor//simulate.py:167:# from .profile import profile_options
flepimop/gempyor_pkg/src/gempyor//simulate.py:276:# @profile_options
flepimop/gempyor_pkg/src/gempyor//simulate.py:277:# @profile()
flepimop/gempyor_pkg/src/gempyor//profile_old.py:9:def profile_options(f):
flepimop/gempyor_pkg/src/gempyor//profile_old.py:13:        "--profile",
flepimop/gempyor_pkg/src/gempyor//profile_old.py:18:        "--profile-output",
flepimop/gempyor_pkg/src/gempyor//profile_old.py:23:        "--profile-sort-by",
flepimop/gempyor_pkg/src/gempyor//profile_old.py:27:        help="Sort the profile stats by total time or cumulative time",
flepimop/gempyor_pkg/src/gempyor//profile_old.py:30:        "--profile-num-stats",
flepimop/gempyor_pkg/src/gempyor//profile_old.py:34:        help="The number of lines of profile stats to print",
flepimop/gempyor_pkg/src/gempyor//profile_old.py:38:        profile = kwargs.pop("profile")
flepimop/gempyor_pkg/src/gempyor//profile_old.py:39:        profile_output = kwargs.pop("profile_output")
flepimop/gempyor_pkg/src/gempyor//profile_old.py:40:        sortby = kwargs.pop("profile_sort_by")
flepimop/gempyor_pkg/src/gempyor//profile_old.py:41:        num_stats = kwargs.pop("profile_num_stats")
flepimop/gempyor_pkg/src/gempyor//profile_old.py:42:        profile = True if profile_output else profile
flepimop/gempyor_pkg/src/gempyor//profile_old.py:43:        if profile:
flepimop/gempyor_pkg/src/gempyor//profile_old.py:47:        if profile:
flepimop/gempyor_pkg/src/gempyor//profile_old.py:53:            if profile_output:
flepimop/gempyor_pkg/src/gempyor//profile_old.py:54:                ps.dump_stats(profile_output)
flepimop/gempyor_pkg/src/gempyor//utils.py:209:def profile(output_file: str = None, sort_by: str = "cumulative", lines_to_print: int = None, strip_dirs: bool = False):
flepimop/gempyor_pkg/src/gempyor//utils.py:211:    A time profiler decorator.
flepimop/gempyor_pkg/src/gempyor//utils.py:212:    Inspired by and modified the profile decorator of Giampaolo Rodola:
flepimop/gempyor_pkg/src/gempyor//utils.py:213:    http://code.activestate.com/recipes/577817-profile-decorator/
flepimop/gempyor_pkg/src/gempyor//utils.py:223:            https://docs.python.org/3/library/profile.html#pstats.Stats.sort_stats
flepimop/gempyor_pkg/src/gempyor//utils.py:236:        >>> @profile(output_file="my_function.prof")
flepimop/gempyor_pkg/src/gempyor//inference.py:520:    # @profile()
flepimop/gempyor_pkg/src/gempyor//calibrate.py:13:# from .profile import profile_options
flepimop/gempyor_pkg/src/gempyor//calibrate.py:112:# @profile_options
flepimop/gempyor_pkg/src/gempyor//calibrate.py:113:# @profile()

Or is this something used on an ad-hoc basis? Does this function actually produce end user value as an ad-hoc function? @jcblemai