Add constructor architecture #1410

mnwhite · 2024-04-15T20:35:08Z

Beginning work on the construct method of AgentType. As of this moment, a basic prototype works for income distributions. Still a lot more to develop and test.

Tests for new functionality/models or Tests to reproduce the bug-fix in code.
Updated documentation of features that add new functionality.
Update CHANGELOG.md with major/minor changes.

Completely untested, will adapt construct_income_process on next commit as test example.

Holy crap, construct() worked on the first try! Only IncShkDstn has been tested, and none of the "error catching" features have been looked at. But hey, it works!

Should pass these tests now, but formatting is probably still wrong.

mnwhite · 2024-04-15T21:59:20Z

As noted in the issue where I proposed this, there are two significant design points to work out:

How to handle situations where it's natural to construct multiple objects in the same function. The most obvious case is the first example I implemented: IncShkDstn, PermShkDstn, and TranShkDstn. I put in an ugly workaround, but something else should be done before this is merged.
Whether and how we want constructors to indicate whether they've made a time-varying or time-invariant object. I'm beginning to come around to the idea that these concepts (to the extent they're in HARK now and will be until several more overhaul steps are completed) should not be intended to be changed at the instance level. Rather, they're a fixed fact of the AgentType. There's a very specific reason they were defined at the instance level, but that reason no longer exists.

Can now collapse a DiscreteDistribution to a single dimension; PermShkDstn and TranShkDstn are made this way now. Moved bespoke constructor dictionary lines out of init. Future plan: each AgentType subclass should have default constructors dictionary in its parameter dictionary.

One test was failing due to lack of seed.

mnwhite · 2024-04-24T16:05:59Z

Can someone tell me how to run the pre-commit routine locally? I can run black, but I don't know how to make lint work.

sbenthall · 2024-04-24T16:09:37Z

$ pre-commit run --all-files

sbenthall · 2024-04-24T16:10:19Z

more info in the developer guide here: https://docs.econ-ark.org/guides/contributing.html#pre-commit-hooks

alanlujan91 · 2024-04-24T16:10:33Z

pip install pre-commit # install 
pre-commit install # start-up
git add * # stage your changes
pre-commit # run the hooks

alanlujan91 · 2024-04-24T16:11:43Z

$ pre-commit run --all-files

you probably don't want to run all files, only your changes

All tests pass except for calc_jacobian, which needs a separate fix.

Missed this one. Jacobian test will still be failing.

All test should pass now. Added new optional input for AgentType.solve() that allows the user to decline to run the presolve routine. For most of our classes, presolve might update the terminal solution and/or run update/constructor methods, but this is fatal to what calc_jacobian wants to do.

I think something about pytest just changed while I was working on this branch, because three tests needed to have a [0] added to them (to reference the only element in the array) in order to be correct. Nothing changed, but the result of these tests did. I'm confused.

One assertEqual test was off by 1e-16 on Python 3.9, but not 3.10. No idea why.

Also added retro support for using [None] rather than None for aXtraExtra.

mnwhite · 2024-04-27T01:30:44Z

A bunch of tests needed syntax changes, but everything passes now. The other things that I'm moving to constructors will be much easier from here on out, and should be completed on Monday.

For this PR, I'm going to set the goal at getting everything in constructor functions that should be (plus some alternates that can be swapped in). After this is merged, in a separate branch/PR, I'm going to tackle undoing/simplifying the custom init methods for each AgentType subclass, and get them all just using AgentType.init ... That also means that the default dictionary should be a class attribute.

alanlujan91 · 2024-04-27T10:47:28Z

We should be doing super().init everywhere

mnwhite · 2024-04-27T15:34:03Z

Even where super().__init__ is used, there can be problems when update() is called by a parent class as part of init before the child class is "ready" for it, leading to unexpected results. In the not-too-distant future, I hope all our AgentType subclasses inherit directly from AgentType, and don't have any special init stuff.

pLvlGrid and pLvlNextFunc are now built with the constructors framework. This means that the *only* difference between the AgentType subclasses in ConsGenIncProcessModel is their default dictionary. GenInc tests pass locally, committing to see what else I broke.

mnwhite · 2024-05-07T21:02:34Z

If tests pass on this commit, the next commit will change exactly one line of code, and revise tests to fit. As is, the "construct pLvlGrid by simulation" method draws on AgentCount, which is fine as long as AgentCount is actually large. The number of eventual simulated agents shouldn't be used in the solution of the problem, even extremely indirectly. So I'm just going to have it use a large number of agents no matter what. This is also why the hardcoded seeds don't matter in this function/method.

Never make last second changes before committing, kids. You will never get it right and only embarrass yourself through the commit history.

…tors

Minor errors in code and dictionaries due to changes in prior commit.

mnwhite · 2024-05-10T21:22:09Z

Still behind schedule, as there were more terminal solvers than I remembered. Still to do the following small tasks:

make some example MrkvArray constructors
a few "alternate example" constructor functions, to demonstrate swappability

sbenthall · 2024-05-13T14:27:59Z

HARK/core.py

+                for j in range(len(args_needed)):
+                    this_arg = args_needed[j]
+                    if hasattr(self, this_arg):
+                        temp_dict[this_arg] = getattr(self, this_arg)


Is there a way to do this without hasattr/getattr/setattr, using manipulations of a data structure rather than an object's attributes?

sbenthall · 2024-05-13T14:30:47Z

HARK/core.py

+                if not any_missing:
+                    temp = constructor(**temp_dict)
+                    setattr(self, key, temp)
+                    self.parameters[key] = temp


Are these parameters, or model constructs, or both?

what if there was a self.constructs dictionary that was separate from parameters?

sbenthall · 2024-05-13T14:34:36Z

HARK/core.py

+        including their names, the function that constructs them, the names of
+        those functions inputs, and whether those inputs are present.
+
+        Parameters


Python conventions for printed representations of things is to create a __repr__ method, or to display docstrings.

sbenthall · 2024-05-13T14:35:31Z

HARK/core.py

+
+        out = ""
+        for i in range(N_keys):
+            key = keys[i]


could try for key in keys: here

I do use i below, because I track which work items have been completed in a boolean vector.

sbenthall · 2024-05-13T14:35:50Z

HARK/core.py

+        out = ""
+        for i in range(N_keys):
+            key = keys[i]
+            has_val = hasattr(self, key)


could be looking up in parameters?

Yes, construct() does this but I forgot to put that line in here. Will fix.

sbenthall · 2024-05-13T14:41:59Z

HARK/core.py

+        Returns
+        -------
+        None.
+        """


This method create functionality that operates on arbitrary functions, since any arbitrary function is a constructor.

Is a constructor a type?

Or a constructor-list a type/data structure with extra functionality?

I think we want to leave it general/simple that any function can be a constructor, to keep barriers to entry low.

Also had to change dictionary import in tests. Notebooks might break.

Too much copy-pasta, something ended up zero that should have been one.

One is an extremely basic binary state specification, the other is a "ratchet" specification in which transitions only go one direction, and only one step at a time.

Same issue as in earlier commit, but in an example notebook. Just imported and used correct dictionary.

Construct method is now more robust, with an option to force through errors and record them in a separate dictionary. Also made other small changes/fixes.

This commit adds one missing (trivial) constructor and adds a few "counterexample" constructors that *could* be used for other parameter specifications. There are many, many more that could be written.

mnwhite · 2024-05-14T19:13:22Z

Once the current tests finish, I'll be pushing a commit that only has documentation / changelog updates. I've added a small number of "alternate" constructors that can be substituted in, but there's an arbitrarily large number of other ones that could be written as well.

Following yesterday's discussion with Seb and Alan, I added an additional features to construct: the option to force it to continue running even if it encounters exceptions. In the default (force=False), any exception other than simply not finding an argument will raise an exception as normal; reaching the end of the method without having completed its work is an exception. Under force=True, it records each exception to _constructor_errors and then ignores it, terminating only when it runs out of work to do, as usual. This option allows the user to (e.g.) run construct(force=True) and then simply examine ThisType._constructor_errors (and do ThisType.describe_constructors()) to get a quick read of what happened when they tried to build all of their constructed inputs.

This PR is ready for others to look at. No external behavior has changed. The only changes in this PR are how complex objects get created.

Also added one missing line of documentation.

sbenthall · 2024-05-15T16:24:20Z

HARK/core.py

+        if param_name in self.parameters:
+            del self.parameters[param_name]
+        if hasattr(self, param_name):
+            delattr(self, param_name)


is there any way to avoid this redundant storage of the constructs?

Maybe constructs should not be stored in parameters, but in self.constructs (or something)

That's for deleting things. Right now, I've written things to be intentionally redundant, because we're "between two worlds". The medium run goal is to remove the top level namespace at all, only putting them in parameters.

I don't want a separate constructs dictionary, because there's no meaningful distinction between parameters and constructs. It might be that parameters was the wrong name for that dictionary.

sbenthall · 2024-05-15T16:26:05Z

This all looks very good to me.

The one thing which I think could be improved is the confusing way in which constructed objects are stored, both (!) as an entry in parameters and as a new attribute on the model object. Neither of these seem quite right to me.

What about a self.constructs dictionary?

sbenthall · 2024-05-15T22:22:21Z

Ok, I understand that removing object attribute storage entirely is a goal, but something to be implemented in a different PR.

I'll leave further notes on #889

sbenthall · 2024-05-21T12:50:44Z

There is a conflicting file with CHANGELOG

Otherwise, I believe this is ready to merge?

mnwhite · 2024-05-21T17:09:33Z

Fixed CHANGELOG

mnwhite added 3 commits April 15, 2024 14:52

First draft of construct() method

a036f24

Completely untested, will adapt construct_income_process on next commit as test example.

IncShkDstn is constructed using construct

4d3e407

Holy crap, construct() worked on the first try! Only IncShkDstn has been tested, and none of the "error catching" features have been looked at. But hey, it works!

Add missing constructor lines to ConsAggShock

0b96804

Should pass these tests now, but formatting is probably still wrong.

mnwhite added 4 commits April 24, 2024 11:16

Remove old lines from ConsAggShockModel

f440295

Turn lists into TimeVaryingDiscreteDistributions

613fa5c

One test was failing due to lack of seed.

Run black on changed files

35cf624

mnwhite added 4 commits April 24, 2024 12:16

Properly run pre-commit

51ed70e

Add aXtraGrid and solution_terminal to constructors

ad6994d

All tests pass except for calc_jacobian, which needs a separate fix.

Fix syntax in asset grid test

3c5b2c7

Missed this one. Jacobian test will still be failing.

This was referenced Apr 26, 2024

IndShockConsumerType.calc_jacobian() changes cFunc_terminal_ #1421

Closed

Why does PerfForesightLabeled inherit from ConsIndShock? #1422

Closed

mnwhite added 5 commits April 26, 2024 17:13

Missed one index spot

bdc2b0f

Change two tests to AlmostEqual

154512d

One assertEqual test was off by 1e-16 on Python 3.9, but not 3.10. No idea why.

Adjust notation in examples

38c376b

Also added retro support for using [None] rather than None for aXtraExtra.

Two more tiny fixes

f1f963e

mnwhite added 2 commits May 7, 2024 16:53

Merge branch 'master' into AddConstructorArchitecture

246874f

mnwhite added 4 commits May 10, 2024 12:39

And typo in dictionary value

e79d786

Never make last second changes before committing, kids. You will never get it right and only embarrass yourself through the commit history.

Moved terminal solution for portfolio and bequest models to constriuc…

a9a9df6

…tors

Fix portfolio and bequest examples

f92b29f

Minor errors in code and dictionaries due to changes in prior commit.

Move ConsLaborModel objects to constructors

22efb70

sbenthall reviewed May 13, 2024

View reviewed changes

mnwhite added 7 commits May 13, 2024 14:47

Move Markov terminal solution to constructor

243bdd7

Also had to change dictionary import in tests. Notebooks might break.

Fix MPC in terminal solution

a8eadab

Too much copy-pasta, something ended up zero that should have been one.

Add two example constructors for MrkvArray

32f3f51

One is an extremely basic binary state specification, the other is a "ratchet" specification in which transitions only go one direction, and only one step at a time.

Fix dictionary import in one example notebook

6df9acb

Same issue as in earlier commit, but in an example notebook. Just imported and used correct dictionary.

Changes weren't saved on prior commit

fa6030a

Added error logging functionality to construct

b282159

Construct method is now more robust, with an option to force through errors and record them in a separate dictionary. Also made other small changes/fixes.

Add a few alternate constructors

357e412

This commit adds one missing (trivial) constructor and adds a few "counterexample" constructors that *could* be used for other parameter specifications. There are many, many more that could be written.

Add CHANGELOG entry

5b9d91d

Also added one missing line of documentation.

mnwhite changed the title ~~WIP: Add constructor architecture~~ Add constructor architecture May 14, 2024

sbenthall reviewed May 15, 2024

View reviewed changes

mnwhite mentioned this pull request May 15, 2024

Flatten default initializer dictionaries #1425

Merged

1 task

sbenthall mentioned this pull request May 15, 2024

the "whiteboard problem" #889

Open

Merge branch 'master' into AddConstructorArchitecture

d82714c

sbenthall merged commit c318576 into master May 21, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add constructor architecture #1410

Add constructor architecture #1410

mnwhite commented Apr 15, 2024 •

edited

mnwhite commented Apr 15, 2024

mnwhite commented Apr 24, 2024

sbenthall commented Apr 24, 2024

sbenthall commented Apr 24, 2024

alanlujan91 commented Apr 24, 2024

alanlujan91 commented Apr 24, 2024

mnwhite commented Apr 27, 2024

alanlujan91 commented Apr 27, 2024

mnwhite commented Apr 27, 2024

mnwhite commented May 7, 2024

mnwhite commented May 10, 2024

sbenthall May 13, 2024

sbenthall May 13, 2024

sbenthall May 13, 2024

sbenthall May 13, 2024

mnwhite May 14, 2024

sbenthall May 13, 2024

mnwhite May 14, 2024

sbenthall May 13, 2024

mnwhite May 14, 2024

mnwhite commented May 14, 2024 •

edited

sbenthall May 15, 2024

mnwhite May 15, 2024

sbenthall commented May 15, 2024

sbenthall commented May 15, 2024

sbenthall commented May 21, 2024

mnwhite commented May 21, 2024

Add constructor architecture #1410

Add constructor architecture #1410

Conversation

mnwhite commented Apr 15, 2024 • edited

mnwhite commented Apr 15, 2024

mnwhite commented Apr 24, 2024

sbenthall commented Apr 24, 2024

sbenthall commented Apr 24, 2024

alanlujan91 commented Apr 24, 2024

alanlujan91 commented Apr 24, 2024

mnwhite commented Apr 27, 2024

alanlujan91 commented Apr 27, 2024

mnwhite commented Apr 27, 2024

mnwhite commented May 7, 2024

mnwhite commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnwhite commented May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbenthall commented May 15, 2024

sbenthall commented May 15, 2024

sbenthall commented May 21, 2024

mnwhite commented May 21, 2024

mnwhite commented Apr 15, 2024 •

edited

mnwhite commented May 14, 2024 •

edited