CDCgov / PyRenew

Python package for multi-signal Bayesian renewal modeling with JAX and NumPyro.
https://cdcgov.github.io/PyRenew/
Apache License 2.0
14 stars 3 forks source link

Document convention of preferring named arguments even for mandatory (positional) args #298

Open AFg6K7h4fhy2 opened 3 months ago

AFg6K7h4fhy2 commented 3 months ago

Given the variety of backgrounds and experiences present across CFA and the fact that CFA members seem, at present time, to constitute most of MSRs early users, I suggest we explicitly write out the argument names for positional arguments.

Pro: The argument names provide useful information to new users of MSR, Python, and certain Python package (e.g., numpyro).

Con: Blurs the lines between required and optional arguments, as now all previously positional arguments are presented as keyword arguments.

Con: More writing for MSR developers.

Examples

I0 = InfectionInitializationProcess(
    "I0_initialization",
    DistributionalRV(dist=dist.LogNormal(2.5, 1), name="I0"),
    InitializeInfectionsZeroPad(pmf_array.size),
    t_unit=1,
)

...would change to

I0 = InfectionInitializationProcess(
    name="I0_initialization",
    I_pre_seed_rv=DistributionalRV(
        dist=dist.LogNormal(loc=2.5, scale=1), name="I0"),
    infection_seed_method=InitializeInfectionsZeroPad(
        n_timepoints=pmf_array.size),
    t_unit=1,
)

Further thoughts are welcome. I do not feel particularly strongly about this, I just believe it might help clarify things to people in the tutorials somewhat.

dylanhmorris commented 3 months ago

I like this suggestion, and in general I like using named arguments for code readability even internally.

dylanhmorris commented 1 month ago

We have moved toward doing this both in code and tutorials. @AFg6K7h4fhy2 would you be willing to add a note to the developer docs recommending this?