light touch Market class refactoring #765

sbenthall · 2020-07-21T16:44:27Z

CDC has asked me to familiarize myself with the Market code because of a certain project.

In this PR, I'll try to do some light maintenance based on previously established design principles.

Some considerations:

A performance benefit for namespaces/dictionaries for state storage rather than as object attributes (see change _hist constructions to history namespace #674)
By packaging alike variable together, it makes it easier to write generalized functions (see grouping exogenous shocks into collection #760 refactor getStates to use a transition function #761)
The more mathematical values can be represented as key/value pairs in a dict-like data structure, the less conflict there is between specialized mathematics formatting desiderata and Python style conventions (see https://github.com/econ-ark/governance/issues/5 )

The current step in this PR, meant to be illustrative, is moving the storage of initial and state values for sow_vars into dictionaries. The trajectory of this work is to make similar adjustments for the reap_vars, etc. The Market class is a nice place to do these changes because the variables are already categorized in the class constructor. (I'd recommend similar changes to the Agent classes, but these are not yet written in as abstract a way.)

The current PR passes the test_ConsAggShockModel.py tests. I'd like to note that this is without changes to e.g. the SmallOpenEconomy that would be necessary to retain functionality. This indicates a gap in the current test suite. If this PR's direction gets approval, I would write tests for those other classes as part of this PR.

sbenthall · 2020-07-21T16:47:36Z

Another reason to store Market state in this way is that this kind of representation is needed by the mill method anyway. Currently, this is done via awkward ad hoc construction. The trajectory of this PR is to have the Market natively store the reap and const variable values in dictionaries, making this ad hoc construction unnecessary.

https://github.com/econ-ark/HARK/blob/master/HARK/core.py#L1058-L1065

sbenthall · 2020-07-23T13:36:53Z

HARK/ConsumptionSaving/ConsAggShockModel.py

@@ -1678,12 +1680,12 @@ def update(self):
        self.KtoLnow_init = self.kSS


did I miss one here? i think so

HARK/core.py

sbenthall · 2020-07-23T13:42:02Z

HARK/core.py

-            setattr(self, this_var, this_product)
+
+        for sow_var in self.sow_state:
+            this_product = getattr(product, sow_var)


This simplifies if millRule returns a dict-like: self.sow_state.update(product)

chose for millRule to return a tuple

(closer to Dolo, supposed to be good for numba)

sbenthall · 2020-07-23T13:43:31Z

HARK/ConsumptionSaving/ConsAggShockModel.py

+        self.sow_init['Mnow'] = self.MSS
+        self.sow_init['Aprev'] = self.KSS
+        self.sow_init['Rnow'] = self.RSS
+        self.sow_init['Wnow'] = self.WSS
        self.PermShkAggNow_init = 1.0


Missed these?

No, not missing because these are not sow_vars in KS

sbenthall · 2020-07-23T20:15:30Z

HARK/ConsumptionSaving/ConsAggShockModel.py

+                           'TranShkAggNow',
+                           'KtoLnow',
+                           'MrkvNow' # This one is new
+                 ],


When all the sow_vars are "Now", "Now" can be left out of the name...

But I don't dare.

sbenthall · 2020-07-23T20:16:36Z

HARK/ConsumptionSaving/ConsAggShockModel.py

@@ -2455,81 +2526,6 @@ def calcAFunc(self, Mnow, Aprev):
        return AggShocksDynamicRule(AFunc_list)


-class CobbDouglasAggVars(HARKobject):


replaced these container classes with tuples same size/order as sow_vars

sbenthall · 2020-07-23T20:18:11Z

HARK/ConsumptionSaving/tests/test_ConsAggShockModel.py

@@ -48,6 +48,9 @@ def test_macro(self):
        self.assertAlmostEqual(self.economy.AFunc.slope,
                               1.116259456228145)

+        self.assertAlmostEqual(self.economy.history['MaggNow'][10],


These tests are added in #773, which should be merged first

sbenthall · 2020-07-23T20:29:04Z

This PR is now ready for review.

llorracc · 2020-07-30T15:47:33Z

Can use regular dicts instead of OrderedDict for Python 3.7+

sbenthall · 2020-07-30T19:50:53Z

0.10.6 supports Python 3.6:
https://github.com/econ-ark/HARK/blob/master/setup.cfg#L42

I think in our meeting today we agreed 0.10.7 would drop Python 3.6 support

sbenthall · 2020-07-30T20:21:01Z

OrderedDict replaced with dict in recent commit

sbenthall · 2020-07-30T20:50:16Z

I expect this to be merged AFTER the 0.10.7 release, since there seems to be a feature freeze there.

* update CHANGELOG with #765 change * Update CHANGELOG.md Co-authored-by: Mridul Seth <seth.mridul+github@gmail.com>

storing sow_var states and initial values in namespace

98fd4a4

project-bot bot added this to Needs Triage in Issues & PRs Jul 21, 2020

sbenthall added 9 commits July 21, 2020 17:12

Merge branch 'master' into sb-market

fddb98d

Merge branch 'master' into sb-market

8feb583

updates to KS model to fit market refactoring

fabec0c

Merge branch 'sb-market' of github.com:sbenthall/HARK into sb-market

f8343cf

Merge branch 'master' into sb-market

9ee55c2

update SmallOpenEconomy

15682cd

const_vars now in dictionary

7444dc5

reap variables to dictionary

e6f1022

collect sow var dicts in with sow vars

0ef77f7

sbenthall commented Jul 23, 2020

View reviewed changes

HARK/core.py Show resolved Hide resolved

sbenthall commented Jul 23, 2020

View reviewed changes

sbenthall added 3 commits July 23, 2020 13:04

adding ConsAggShockModel tests for Market history tracking

b7b91ab

fixing trak_vars

7b78cd7

millRule returning a tuple of values same size/order as sow_vars

2220fa5

sbenthall commented Jul 23, 2020

View reviewed changes

sbenthall added 3 commits July 23, 2020 16:22

Merge branch 'master' into sb-market

206ca76

missed a sow_var in Small Open

3b678d1

Merge branch 'sb-market' of github.com:sbenthall/HARK into sb-market

7694a56

sbenthall requested a review from mnwhite July 23, 2020 20:28

sbenthall changed the title ~~[WIP] light touch Market class refactoring~~ light touch Market class refactoring Jul 23, 2020

Merge branch 'master' into sb-market

7e45b5b

sbenthall added 2 commits July 30, 2020 16:13

replace OrderedDict with dict -- 3.7 dicts ordered by default

a6e2de9

Merge branch 'sb-market' of github.com:sbenthall/HARK into sb-market

3237b56

sbenthall added the Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged label Jul 30, 2020

Merge branch 'master' into sb-market

9cc7428

MridulS removed the Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged label Aug 5, 2020

sbenthall and others added 3 commits August 5, 2020 13:53

Merge branch 'master' into sb-market

85f204c

Merge branch 'master' into sb-market

6c31ea8

Merge branch 'master' into sb-market

f812249

llorracc added the Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged label Aug 13, 2020

sbenthall merged commit 1d15c4d into econ-ark:master Aug 13, 2020

Issues & PRs automation moved this from Needs Triage to Done Aug 13, 2020

sbenthall added a commit that referenced this pull request Aug 13, 2020

update CHANGELOG with #765 change

83992c4

sbenthall mentioned this pull request Aug 13, 2020

update CHANGELOG with #765 change #796

Merged

MridulS pushed a commit that referenced this pull request Aug 14, 2020

update CHANGELOG with #765 change (#796)

35ad3e8

* update CHANGELOG with #765 change * Update CHANGELOG.md Co-authored-by: Mridul Seth <seth.mridul+github@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

light touch Market class refactoring #765

light touch Market class refactoring #765

sbenthall commented Jul 21, 2020

sbenthall commented Jul 21, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall Jul 23, 2020

sbenthall commented Jul 23, 2020

llorracc commented Jul 30, 2020

sbenthall commented Jul 30, 2020

sbenthall commented Jul 30, 2020

sbenthall commented Jul 30, 2020

		@@ -1678,12 +1680,12 @@ def update(self):
		self.KtoLnow_init = self.kSS

		@@ -2455,81 +2526,6 @@ def calcAFunc(self, Mnow, Aprev):
		return AggShocksDynamicRule(AFunc_list)


		class CobbDouglasAggVars(HARKobject):

light touch Market class refactoring #765

light touch Market class refactoring #765

Conversation

sbenthall commented Jul 21, 2020

sbenthall commented Jul 21, 2020

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

Choose a reason for hiding this comment

sbenthall commented Jul 23, 2020

llorracc commented Jul 30, 2020

sbenthall commented Jul 30, 2020

sbenthall commented Jul 30, 2020

sbenthall commented Jul 30, 2020