Skip to content

forecasting package: code sanding #281

@JavadocMD

Description

@JavadocMD

A number of minor matters of code style and conventions worth addressing during the forecast beta:

  • Console output while running (for Filters and Forecast) should be stylistically aligned with BasicSimulator: controlled by a logging context, and consider displaying progress bar and other status info as makes sense.
  • There are several cases where the code is applying a mapping function the values of a dict while keeping the keys the same, but it's using a loop over another set of keys. (e.g., here) This isn't a bug because we (with general knowledge of the program) know that the set of keys is the same based on how the dict was constructed, however I would argue this is slightly more complex than necessary (harder to read and a possible source of bug-by-modification). Ultimately safer to use the keys of the dict you're mapping. Also consider using our util function map_values().
  • Overridden methods should be annotated @override
  • Classes with abstract members should extend ABC (e.g., Prior)
  • Parameters and return values should be type annotated
  • Classes should declare all class attributes with types (ExponentialTransform and ShiftTransform don't)
  • Prefer being explicit with numpy data-type type annotations (e.g., NDArray can usually be NDArray[np.int64] or similar); some of these can get very tricky but I can most of the time figure out a way. All else fails, docstrings should mention the intended data-type bounds.
  • All modules should have a top-level comment
  • All public functions/classes have docstrings
  • Internal functions/classes don't need detailed docstrings, but it's nice just to have a line or two of general description (_initialize_compartments_and_params() for example)
  • There are a few instances of docstring style inconsistency (w.r.t. the rest of epymorph) like doc'ing class Attributes instead of Parameters for dataclasses, etc.
  • Parameter docstrings can often skip the type annotation when that's provided in the method signature
  • Avoid pre-3.9 type annotations (e.g., Tuple[int, ...] should be tuple[int, ...])
  • Could Prior instances all be dataclasses for consistency?
  • Might consider replacing ParamFunctionDynamics' with_initial() by making these dataclasses and relying on the built-in replace -- I get the impression this was an effort to separate what the user specifies vs. what the internal logic specified, but I wonder if a more "standard" approach might be preferable here. (There might be conflicts here with the ParamFunction metaclass, but I want to factor that out with __init_subclass__ anyway...)
  • I think EnsembleKalmanFilterSimulator could check its Gaussian invariant at construction time rather than waiting for run; preferable to fail early.
  • Several instances of typo: "voliatility" vs. "volatility"

There are also some possible Quality of Life improvements for the rest of epymorph which might be useful for forecasting:

  • Should RandomLocationsAndRandomSeedInitializer (or equivalent) be a library initializer?
  • time_frame.split(...) function that takes N varargs where each is a number of days, a date, or a string parse-able as date and returns a tuple of N+2 TimeFrames (as long as all the splits are valid)
  • GeoScope should support the len() function.
  • DateRange and TimeFrame should support in operator.

Metadata

Metadata

Labels

No labels
No labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions