Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repeated logic in several modules #480

Open
QBatista opened this issue Mar 19, 2019 · 1 comment
Open

Repeated logic in several modules #480

QBatista opened this issue Mar 19, 2019 · 1 comment

Comments

@QBatista
Copy link
Member

There are several existing classes that appear to be reproducing closely related functionalities, thus violating the "Don't Repeat Yourself" principle which is outlined in one of the lectures in one of the lectures. For instance, all the following modules include a form of a linear state transition equation

  • robustlq.py
  • lss.py
  • lqnash.py
  • lqcontrol.py
  • army.py
  • kalman.py

#476 introduces yet another class which falls into this group. The ARMA class has a simulation method, the LQ class has a compute_sequence method and the LinearStateSpace has a simulate method. Besides, both Kalman and LQ have stationary_values methods that use solve_discrete_riccati. Similarly, LinearStateSpace and ARMA have impulse_response methods.

Therefore, we might want to consider refactoring these classes, potentially into a sub-package. Besides avoiding repetition and making it easier to understand how these classes are connected, another benefit would be that features added to a base class would directly propagate to derived classes.

As @thomassargent30 pointed out, this refactoring is bound to take some time and require a person who is familiar with all of the lectures that use these classes. Considering that some new lectures which use these classes are currently in production, @jstac suggested that we hold off on a major reorganization and instead consider piecemeal improvements. Specifically, we could add a linear_gaussian module that contains:

  1. a jitted function that simulates vector-valued Gaussian processes, and

  2. a jitted function that returns the stationary mean and variance.

Does anyone have any additional thoughts or suggestions?

@jstac
Copy link
Contributor

jstac commented Mar 19, 2019

I think it makes sense to collect these modules into a subpackage, even though it will break some users' code. Since subpackage names are best kept short, I propose linmod.

Regarding the linear_gaussian module mentioned above, which should live inside the subpackage, the function that returns stationary mean and variance doesn't actually care whether the shocks are Gaussian, so I propose we instead call it linear_dynamics.

I tend to prefer flat structures over nested when we're dealing with relatively small modules, so I would want to see a strong case for subclassing before we went down that road.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants