Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
hmgaudecker
left a comment
hmgaudecker
left a comment
There was a problem hiding this comment.
Very nice!!!
Most of my comments are probably just me not getting the line between deletion and later refactoring right.
| @@ -213,31 +140,19 @@ def _segment_logsumexp(a, segment_info): | |||
| def _determine_dense_discrete_choice_axes( | |||
There was a problem hiding this comment.
| def _determine_dense_discrete_choice_axes( | |
| def _determine_discrete_choice_axes( |
Not sure whether getting rid of dense terminology is out-of-scope for this PR or not. But might add clarity.
There was a problem hiding this comment.
I would say it is out-of-scope. The problem is that we still need --what we currently call sparse variables-- for the simulation. I would like to avoid a state of the code-base where we have "variables" and "sparse variables" to avoid confusion. In the upcoming renaming/refactoring PR I am planning to give these two concepts better fitting names.
hmgaudecker
left a comment
hmgaudecker
left a comment
There was a problem hiding this comment.
As discussed, looks great with the possible exception of recycling the code from test_state_space !
2 participants
In this PR, we remove filters from the code-base.
We remove filters and related code from the code-base without starting a major refactoring. This will be done in a separate PR.
Note
Sparse variables are still required in the simulation; however, no filters are needed and only state variables can be sparse not choices. I will think about a better name for "sparse" variables in this context in the next PRs.
ToDo in next PR's
Refactoring and Renaming
_determine_dense_discrete_choice_axesdiscrete_problem._determine_dense_discrete_choice_axesandsimulate.determine_discrete_dense_choice_axes: They look similar, and do similar things, but not exactly -> Combine these with an argument likepurpose: Literal["solution", "simulation"].