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.
A number of minor matters of code style and conventions worth addressing during the forecast beta:
map_values().@overrideABC(e.g., Prior)NDArraycan usually beNDArray[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.Tuple[int, ...]should betuple[int, ...])with_initial()by making these dataclasses and relying on the built-inreplace-- 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...)There are also some possible Quality of Life improvements for the rest of epymorph which might be useful for forecasting:
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)len()function.inoperator.