-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow general discrete grids #90
Conversation
…conomics/lcm into update-process-model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start, path forward as discussed in person!
Should provide only the following option to create discrete grids: @dataclass
class RetirementStatus:
working: int = 0
retired: int = 1
DiscreteGrid(RetirementStatus) |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, really nice!
…nSourceEconomics/lcm into allow-general-discrete-grids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a brief look -- very nice! Need to stop now.
Apologies for yet another round of renaming suggestions, but looking at it I wasn't certain that a newcomer would immediately understand what is expected where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I really see how the OOP features are beneficial.
Just a few questions or comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks!
return make_dataclass("CategoryClass", init) | ||
|
||
|
||
@pytest.fixture(scope="session") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check whether you really need the two functions here? I.e., with and without underscore? Can't we just decorate the function itself or does that lead to issues because it takes an argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the argument becomes a problem. While pytest supports fixtures that have arguments, it seems like you have to parametrize it via the parametrize decorator, and that would not work in our case, because we need to use different versions of it in the same test.
In this PR, I add the functionality to allow general discrete grids.
This entails two major updates:
DiscreteGrid
with a dataclass that provides a mapping between a category name and its code. See the new syntax below.Closes #82, #87.
New Syntax
Note
For most use cases, the codes (the field values of the dataclass) should be indices; however, with this PR it is now possible to use non-index codes; we therefore showcase this feature below