mnwhite
commented
4 years ago Yes, I coded it this way to allow for time-varying solveOnePeriod functions. This was intentional.
Not all of our solution functions create a solver class. Some of them are just coded as a single function, which is a lot easier to read and write.
I don't particularly like our "object oriented solvers". The idea was to not rewrite code across solvers, but they made the code an absolute maze to navigate. Moreover, we never actually had a meeting to plan out what the scope of models that HARK should be able to handle (but not for lack of trying), so the "forward looking" structure of the solvers wasn't particularly forward looking.
As for redundant working being done by instantiating and then destroying solver objects: yes, 100%, and there's an old issue that I wrote years ago about this. I think you can find it if you search for "persistent solvers". The idea is to have a better partition between objects that can be re-used across periods of the model and those that need to be created every time.
On Fri, Apr 24, 2020, 7:17 PM Sebastian Benthall notifications@github.com wrote:
There is a strange coding pattern in the Model classes. I am tempted to try to turn it into something more standard, but want to check if there is a hidden purpose to it.
Typically, a ConsumerType will have an attribute, solveOnePeriod, that is set upon initialization to some stand-alone function in the module. E.g.:
https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsIndShockModel.py#L1658
This stand alone method then typically initalizes a Solver object, runs solve() on it, then returns the result (a solution). E.g.:
https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsIndShockModel.py#L590-L625
These methods are for the most part, except for the choice of Solver class, identical across the library.
This is not ideal because it makes it difficult to read the code: to follow the solution logic, you have to jump out of one class into the global scope, then into another.
It seems to me that either:
- (A) solveOnePeriod could be a method, which would just look like exactly the function that is currently assigned to it, or
- (B) solveOnePeriod could be a standardized method, which points to a particular Solver class, which is then parameterized in the model code.
The only motivation I can think of for the current implementation is that it's suggested by this code block that the idea was to make it possible to have time-varying solution functions:
However, this flexibility does not seem to be used anywhere in the HARK repository (I checked), nor does it have any tests associated with it. If this was still a desirable feature, it would be possible to build 'solveOnePeriod', as a standardized method, to optionally take a list of class names that could be indexed by time periods.
[Separately, and maybe to be addressed in a different issue: I'm skeptical about the performance of this method of firing up a new Solver object, complete with a lot of initialization, at every step of the backwards inductions. It seems to me there may be a lot of redundant code calls. I'll revisit this once I've got a profiler set up, but I wanted to mention it now to start circulating the thought.]
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/658, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFPJRRIYWVBVYTJMUU3ROIMWZANCNFSM4MQPH6XQ .
sbenthall
llorracc
There is a strange coding pattern in the Model classes. I am tempted to try to turn it into something more standard, but want to check if there is a hidden purpose to it.
Typically, a ConsumerType will have an attribute,
solveOnePeriod, that is set upon initialization to some stand-alone function in the module. E.g.:https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsIndShockModel.py#L1658
This stand alone method then typically initalizes a Solver object, runs
solve()on it, then returns the result (a solution). E.g.:https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsIndShockModel.py#L590-L625
These methods are for the most part, except for the choice of Solver class, identical across the library.
This is not ideal because it makes it difficult to read the code: to follow the solution logic, you have to jump out of one class into the global scope, then into another.
It seems to me that either:
The only motivation I can think of for the current implementation is that it's suggested by this code block that the idea was to make it possible to have time-varying solution functions:
https://github.com/econ-ark/HARK/blob/78d1981233a6cda398d4e3007be03f558a7bc910/HARK/core.py#L807-L810
However, this flexibility does not seem to be used anywhere in the HARK repository (I checked), nor does it have any tests associated with it. If this was still a desirable feature, it would be possible to build 'solveOnePeriod', as a standardized method, to optionally take a list of class names that could be indexed by time periods.
[Separately, and maybe to be addressed in a different issue: I'm skeptical about the performance of this method of firing up a new Solver object, complete with a lot of initialization, at every step of the backwards inductions. It seems to me there may be a lot of redundant code calls. I'll revisit this once I've got a profiler set up, but I wanted to mention it now to start circulating the thought.]