-
-
Notifications
You must be signed in to change notification settings - Fork 204
[WIP] Abstract Variables/Symbols #1308
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
base: main
Are you sure you want to change the base?
Changes from all commits
a7ea228
19ceb72
f41c944
c523d4b
ccdf96e
207696f
918177d
376a49a
64e4c09
e4d2850
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| --- | ||
| states: !StateSpace | ||
| variables: | ||
| - !State | ||
| name: m | ||
| short_name: money | ||
| long_name: market resources | ||
| latex_repr: \mNrm | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know how you're imagining using the Latex representation.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now these are filler, I want to throw any non-required keys into an attributes dictionary. |
||
| - !State | ||
| name: &name stigma | ||
| short_name: &short_name risky share | ||
| long_name: &long_name risky share of portfolio | ||
| latex_repr: &latex_repr \stigma | ||
|
|
||
| actions: !ActionSpace | ||
| variables: | ||
| - !Action | ||
| name: c | ||
| short_name: consumption | ||
| long_name: consumption | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these fields optional? Can one spill over to the others as a default?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are optional, only required is name. |
||
| latex_repr: \cNrm | ||
| - !Action | ||
| name: *name | ||
| short_name: *short_name | ||
| long_name: *long_name | ||
| latex_repr: *latex_repr | ||
|
|
||
| post_states: !PostStateSpace | ||
| variables: | ||
| - !PostState | ||
| name: a | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like you've repeated the post_states block twice in this file? Not sure how @mnwhite feels, but maybe we don't need to draw a firm distinction between states and post states like this. Or the labels could be inside the variable, not part of the document structure. Compare: with The latter is same information, but fewer lines. |
||
| short_name: assets | ||
| long_name: savings | ||
| latex_repr: \aNrm | ||
| - !PostState | ||
| name: *name | ||
| short_name: *short_name | ||
| long_name: *long_name | ||
| latex_repr: *latex_repr | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| --- | ||
| states: !StateSpace | ||
| variables: | ||
| - !State | ||
| name: m | ||
| short_name: money | ||
| long_name: market resources | ||
| latex_repr: \mNrm | ||
| - !State | ||
| name: &name stigma | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what the ampersands are doing here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ampersands are aliases, since one of the states is also a control and a post-state. I will document this more as I make more progress. |
||
| short_name: &short_name risky share | ||
| long_name: &long_name risky share of portfolio | ||
| latex_repr: &latex_repr \stigma | ||
|
|
||
| actions: !ActionSpace | ||
| variables: | ||
| - !Action | ||
| name: c | ||
| short_name: consumption | ||
| long_name: consumption | ||
| latex_repr: \cNrm | ||
| - !Action | ||
| name: *name | ||
| short_name: *short_name | ||
| long_name: *long_name | ||
| latex_repr: *latex_repr | ||
|
|
||
| post_states: !PostStateSpace | ||
| variables: | ||
| - !PostState | ||
| name: a | ||
| short_name: assets | ||
| long_name: savings | ||
| latex_repr: \aNrm | ||
| - !PostState | ||
| name: *name | ||
| short_name: *short_name | ||
| long_name: *long_name | ||
| latex_repr: *latex_repr | ||
|
|
||
| parameters: !Parameters | ||
| variables: | ||
| - !Parameter | ||
| name: DiscFac | ||
| short_name: discount factor | ||
| long_name: discount factor | ||
| latex_repr: \beta | ||
| - !Parameter | ||
| name: CRRA | ||
| short_name: risk aversion | ||
| long_name: coefficient of relative risk aversion | ||
| latex_repr: \rho | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import unittest | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| import numpy as np | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| import numpy as np |
Copilot
AI
Jan 28, 2026
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.
Import of 'HARK' is not used.
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.
A test case showing how this data structure could be used in practice would go a long way.
Copilot
AI
Jan 28, 2026
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.
The test data path is hard-coded as a relative string ("HARK/abstract/tests/"), which depends on the test runner's current working directory. Consider deriving paths from __file__ (e.g., via pathlib.Path) so the tests are robust when run from other working directories.
Copilot
AI
Jan 28, 2026
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.
These tests only load YAML but don't assert anything about the parsed result (e.g., that the returned objects are StateSpace/ActionSpace, that variables are present, etc.). Since this PR introduces YAML-backed variable abstractions, adding a few assertions would make the tests actually validate behavior rather than just “no exception.”
Copilot
AI
Jan 28, 2026
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.
Variable data is not used.
| data = yaml.safe_load(f) | |
| def test_full(self): | |
| with open(self.path + "consindshk_full.yml") as f: | |
| data = yaml.safe_load(f) | |
| data = yaml.safe_load(f) | |
| self.assertIsNotNone(data) | |
| def test_full(self): | |
| with open(self.path + "consindshk_full.yml") as f: | |
| data = yaml.safe_load(f) | |
| self.assertIsNotNone(data) |
Copilot
AI
Jan 28, 2026
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.
Variable data is not used.
| data = yaml.safe_load(f) | |
| def test_full(self): | |
| with open(self.path + "consindshk_full.yml") as f: | |
| data = yaml.safe_load(f) | |
| data = yaml.safe_load(f) | |
| self.assertIsNotNone(data) | |
| def test_full(self): | |
| with open(self.path + "consindshk_full.yml") as f: | |
| data = yaml.safe_load(f) | |
| self.assertIsNotNone(data) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,277 @@ | ||||||||||||||||||||||||||||||||
| from dataclasses import dataclass, field | ||||||||||||||||||||||||||||||||
| from typing import Mapping, Optional, Union | ||||||||||||||||||||||||||||||||
| from warnings import warn | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| import numpy as np | ||||||||||||||||||||||||||||||||
| import xarray as xr | ||||||||||||||||||||||||||||||||
| from yaml import SafeLoader, YAMLObject | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| rng = np.random.default_rng() | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| @dataclass | ||||||||||||||||||||||||||||||||
| class Variable(YAMLObject): | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to initialize Variables without parsing them from a YAML file? I'm a little wary of tying the model objects too tightly to the serial format because it can make it tricky to interoperate with other python libraries. |
||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||
| Abstract class for representing variables. Variables are the building blocks | ||||||||||||||||||||||||||||||||
| of models. They can be parameters, states, actions, shocks, or auxiliaries. | ||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| name: str # The name of the variable, required | ||||||||||||||||||||||||||||||||
| attrs: dict = field(default_factory=dict, kw_only=True) | ||||||||||||||||||||||||||||||||
| short_name: str = field(default=None, kw_only=True) | ||||||||||||||||||||||||||||||||
| long_name: str = field(default=None, kw_only=True) | ||||||||||||||||||||||||||||||||
| latex_repr: str = field(default=None, kw_only=True) | ||||||||||||||||||||||||||||||||
| yaml_tag: str = field(default="!Variable", kw_only=False) | ||||||||||||||||||||||||||||||||
| yaml_loader = SafeLoader | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
Comment on lines
+19
to
+26
|
||||||||||||||||||||||||||||||||
| def __post_init__(self): | ||||||||||||||||||||||||||||||||
| for key in ["long_name", "short_name", "latex_repr"]: | ||||||||||||||||||||||||||||||||
| self.attrs.setdefault(key, None) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+28
to
+29
|
||||||||||||||||||||||||||||||||
| for key in ["long_name", "short_name", "latex_repr"]: | |
| self.attrs.setdefault(key, None) | |
| # Synchronize metadata between top-level fields and attrs. | |
| for key in ["long_name", "short_name", "latex_repr"]: | |
| field_value = getattr(self, key) | |
| # If the dataclass field is set, it is the source of truth. | |
| if field_value is not None: | |
| self.attrs[key] = field_value | |
| else: | |
| # Otherwise, if attrs already has a non-None value, mirror it back. | |
| if key in self.attrs and self.attrs[key] is not None: | |
| setattr(self, key, self.attrs[key]) | |
| else: | |
| # Ensure the key exists in attrs for downstream consumers. | |
| self.attrs.setdefault(key, None) |
Copilot
AI
Jan 28, 2026
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.
This module uses PEP 585 built-in generics like list[Variable] / list[str], which are not valid on Python 3.8 without from __future__ import annotations. The project declares requires-python = ">=3.8" in pyproject.toml, so this will break on supported versions unless you either (a) add the future import, (b) switch to typing.List[...], or (c) bump the minimum Python version everywhere (metadata + CI).
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.
I agree with Chris that 'auxiliary' is more like a macro.
I wouldn't use 'auxiliary' here, though it makes sense to have this sort of object.
Copilot
AI
Jan 28, 2026
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.
State.assign_values returns a new Dataset but does not store it on the instance; State.discretize calls self.assign_values(...) and ignores the return value. As written, discretize has no effect on the State. Either have assign_values mutate self.array/self.domain etc., or have discretize return the created Dataset and update callers accordingly.
Copilot
AI
Jan 28, 2026
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.
The docstring includes placeholder text (Args: Variable (_type_): _description_) which is misleading/inaccurate for a public class. Please either remove the placeholder section or replace it with the actual arguments/meaning.
Copilot
AI
Jan 28, 2026
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.
ActionSpace.actions is annotated as Mapping[str, State], but it is populated from self.variables which will contain Action instances. This should be Mapping[str, Action] (or a common base type) to avoid type/usage errors.
Copilot
AI
Jan 28, 2026
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.
The Shock class docstring also contains placeholder boilerplate (Args: Variable (_type_): _description_), which is misleading for users. Please remove or replace it with accurate documentation.
Copilot
AI
Jan 28, 2026
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.
ShockSpace.shocks is declared as list[Shock], but VariableSpace.__post_init__ converts variables into a dict and ShockSpace.__post_init__ assigns that dict to self.shocks. This mismatch will break expectations for iteration/order and static typing. Consider making shocks a Mapping[str, Shock] (consistent with the assigned value) or avoid converting to a dict.
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.
Is this method duplicated?
Copilot
AI
Jan 28, 2026
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.
make_states_array leaves values_len undefined when values is neither list nor np.ndarray, which will raise an UnboundLocalError later. Add an explicit else that raises a clear TypeError/ValueError, or normalize inputs up front.
Copilot
AI
Jan 28, 2026
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.
attrs = attrs or [{}] * values_len reuses the same dict instance for every state, so mutating one state's attrs would affect all others. Use a list comprehension to create distinct dicts per state.
| attrs = attrs or [{}] * values_len | |
| attrs = attrs or [{} for _ in range(values_len)] |
Copilot
AI
Jan 28, 2026
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.
return xr.merge([states]) passes a nested list (states is already a list of Datasets), which will error or produce an unexpected merge. It should merge the list directly (and likely states should be the list of datasets).
| return xr.merge([states]) | |
| return xr.merge(states) |
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.
CI now only tests Python 3.10, but
pyproject.tomldeclaresrequires-python = ">=3.8"and classifiers include 3.8/3.9. This change can mask compatibility regressions (and this PR introduceslist[...]type hints that will break on 3.8). Either restore 3.8/3.9 in the matrix or update the project's supported Python versions consistently (metadata + docs + code).