Merge pull request #1582 from martinholmer/resolve-issue-304

Resolve issue 304 regarding Calculator constructor logic

RELEASES.md

@@ -19,6 +19,9 @@ Release 0.12.0 on 2017-??-??
 - Rename dropq as tbi (taxbrain interface) and refactor run_nth_year_*_model functions so that either puf.csv or cps.csv can be used as input data
   [[#1577](https://github.com/open-source-economics/Tax-Calculator/pull/1577)
   by Martin Holmer]
+- Change Calculator class constructor so that it makes a deep copy of each specified object for internal use
+  [[#1582](https://github.com/open-source-economics/Tax-Calculator/pull/1582)
+  by Martin Holmer]
 
 **New Features**
 - Add Calculator.reform_documentation that generates plain text documentation of a reform

taxcalc/calculate.py

@@ -41,47 +41,29 @@ class Calculator(object):
     Parameters
     ----------
     policy: Policy class object
-        this argument must be specified
-        IMPORTANT NOTE: never pass the same Policy object to more than one
-        Calculator.  In other words, when specifying more
-        than one Calculator object, do this::
-
-            pol1 = Policy()
-            rec1 = Records()
-            calc1 = Calculator(policy=pol1, records=rec1)
-            pol2 = Policy()
-            rec2 = Records()
-            calc2 = Calculator(policy=pol2, records=rec2)
+        this argument must be specified and object is copied for internal use
 
     records: Records class object
-        this argument must be specified
-        IMPORTANT NOTE: never pass the same Records object to more than one
-        Calculator.  In other words, when specifying more
-        than one Calculator object, do this::
-
-            pol1 = Policy()
-            rec1 = Records()
-            calc1 = Calculator(policy=pol1, records=rec1)
-            pol2 = Policy()
-            rec2 = Records()
-            calc2 = Calculator(policy=pol2, records=rec2)
+        this argument must be specified and object is copied for internal use
 
     verbose: boolean
         specifies whether or not to write to stdout data-loaded and
         data-extrapolated progress reports; default value is true.
 
     sync_years: boolean
-        specifies whether or not to syncronize policy year and records year;
+        specifies whether or not to synchronize policy year and records year;
         default value is true.
 
     consumption: Consumption class object
         specifies consumption response assumptions used to calculate
         "effective" marginal tax rates; default is None, which implies
-        no consumption responses assumed in marginal tax rate calculations.
+        no consumption responses assumed in marginal tax rate calculations;
+        when argument is an object it is copied for internal use
 
     behavior: Behavior class object
-        specifies behaviorial responses used by Calculator; default is None,
-        which implies no behavioral responses to policy reform.
+        specifies behavioral responses used by Calculator; default is None,
+        which implies no behavioral responses to policy reform;
+        when argument is an object it is copied for internal use
 
     Raises
     ------
@@ -91,25 +73,37 @@ class Calculator(object):
     Returns
     -------
     class instance: Calculator
+
+    Notes
+    -----
+    The most efficient way to specify current-law and reform Calculator
+    objects is as follows:
+         pol = Policy()
+         rec = Records()
+         calc1 = Calculator(policy=pol, records=rec)  # current-law
+         pol.implement_reform(...)
+         calc2 = Calculator(policy=pol, records=rec)  # reform
+    All calculations are done on the internal copies of the Policy and
+    Records objects passed to each of the two Calculator constructors.
     """
 
     def __init__(self, policy=None, records=None, verbose=True,
                  sync_years=True, consumption=None, behavior=None):
         # pylint: disable=too-many-arguments,too-many-branches
         if isinstance(policy, Policy):
-            self.policy = policy
+            self.policy = copy.deepcopy(policy)
         else:
             raise ValueError('must specify policy as a Policy object')
         if isinstance(records, Records):
-            self.records = records
+            self.records = copy.deepcopy(records)
         else:
             raise ValueError('must specify records as a Records object')
         if self.policy.current_year < self.records.data_year:
             self.policy.set_year(self.records.data_year)
         if consumption is None:
             self.consumption = Consumption(start_year=policy.start_year)
         elif isinstance(consumption, Consumption):
-            self.consumption = consumption
+            self.consumption = copy.deepcopy(consumption)
             while self.consumption.current_year < self.policy.current_year:
                 next_year = self.consumption.current_year + 1
                 self.consumption.set_year(next_year)
@@ -118,7 +112,7 @@ def __init__(self, policy=None, records=None, verbose=True,
         if behavior is None:
             self.behavior = Behavior(start_year=policy.start_year)
         elif isinstance(behavior, Behavior):
-            self.behavior = behavior
+            self.behavior = copy.deepcopy(behavior)
             while self.behavior.current_year < self.policy.current_year:
                 next_year = self.behavior.current_year + 1
                 self.behavior.set_year(next_year)

taxcalc/tests/test_behavior.py

@@ -5,7 +5,7 @@
 from taxcalc import Policy, Records, Calculator, Behavior
 
 
-def test_incorrect_Behavior_instantiation():
+def test_incorrect_behavior_instantiation():
     with pytest.raises(ValueError):
         Behavior(behavior_dict=list())
     bad_behv_dict = {
@@ -37,74 +37,83 @@ def test_incorrect_Behavior_instantiation():
         Behavior(behavior_dict=bad_behv_dict)
 
 
-def test_behavioral_response_Calculator(cps_subsample):
-    # create Records objects
-    records_x = Records.cps_constructor(data=cps_subsample)
-    records_y = Records.cps_constructor(data=cps_subsample)
-    year = records_x.current_year
-    # create Policy objects
-    policy_x = Policy()
-    policy_y = Policy()
-    # implement policy_y reform
+def test_behavioral_response_calculator(cps_subsample):
+    # create Records object
+    rec = Records.cps_constructor(data=cps_subsample)
+    year = rec.current_year
+    # create Policy object
+    pol = Policy()
+    # create current-law Calculator object
+    calc1 = Calculator(policy=pol, records=rec)
+    # implement policy reform
     reform = {year: {'_II_rt7': [0.496],
                      '_PT_rt7': [0.496]}}
-    policy_y.implement_reform(reform)
-    # create two Calculator objects
-    behavior_y = Behavior()
-    calc_x = Calculator(policy=policy_x, records=records_x)
-    calc_y = Calculator(policy=policy_y, records=records_y,
-                        behavior=behavior_y)
+    pol.implement_reform(reform)
+    # create reform Calculator object with no behavioral response
+    behv = Behavior()
+    calc2 = Calculator(policy=pol, records=rec, behavior=behv)
     # test incorrect use of Behavior._mtr_xy method
     with pytest.raises(ValueError):
-        behv = Behavior._mtr_xy(calc_x, calc_y,
-                                mtr_of='e00200p',
-                                tax_type='nonsense')
-    # vary substitution and income effects in calc_y
+        Behavior._mtr_xy(calc1, calc2, mtr_of='e00200p', tax_type='nonsense')
+    # vary substitution and income effects in Behavior object
     behavior0 = {year: {'_BE_sub': [0.0],
                         '_BE_cg': [0.0],
                         '_BE_charity': [[0.0, 0.0, 0.0]]}}
-    behavior_y.update_behavior(behavior0)
-    calc_y_behavior0 = Behavior.response(calc_x, calc_y)
+    behv0 = Behavior()
+    behv0.update_behavior(behavior0)
+    calc2 = Calculator(policy=pol, records=rec, behavior=behv0)
+    assert calc2.behavior.has_response() is False
+    calc2_behv0 = Behavior.response(calc1, calc2)
     behavior1 = {year: {'_BE_sub': [0.3], '_BE_inc': [-0.1], '_BE_cg': [0.0],
                         '_BE_subinc_wrt_earnings': [True]}}
-    behavior_y.update_behavior(behavior1)
-    assert behavior_y.has_response() is True
+    behv1 = Behavior()
+    behv1.update_behavior(behavior1)
+    calc2 = Calculator(policy=pol, records=rec, behavior=behv1)
+    assert calc2.behavior.has_response() is True
     epsilon = 1e-9
-    assert abs(behavior_y.BE_sub - 0.3) < epsilon
-    assert abs(behavior_y.BE_inc - -0.1) < epsilon
-    assert abs(behavior_y.BE_cg - 0.0) < epsilon
-    calc_y_behavior1 = Behavior.response(calc_x, calc_y)
+    assert abs(calc2.behavior.BE_sub - 0.3) < epsilon
+    assert abs(calc2.behavior.BE_inc - -0.1) < epsilon
+    assert abs(calc2.behavior.BE_cg - 0.0) < epsilon
+    calc2_behv1 = Behavior.response(calc1, calc2)
     behavior2 = {year: {'_BE_sub': [0.5], '_BE_cg': [-0.8]}}
-    behavior_y.update_behavior(behavior2)
-    calc_y_behavior2 = Behavior.response(calc_x, calc_y)
+    behv2 = Behavior()
+    behv2.update_behavior(behavior2)
+    calc2 = Calculator(policy=pol, records=rec, behavior=behv2)
+    assert calc2.behavior.has_response() is True
+    calc2_behv2 = Behavior.response(calc1, calc2)
     behavior3 = {year: {'_BE_inc': [-0.2], '_BE_cg': [-0.8]}}
-    behavior_y.update_behavior(behavior3)
-    calc_y_behavior3 = Behavior.response(calc_x, calc_y)
+    behv3 = Behavior()
+    behv3.update_behavior(behavior3)
+    calc2 = Calculator(policy=pol, records=rec, behavior=behv3)
+    assert calc2.behavior.has_response() is True
+    calc2_behv3 = Behavior.response(calc1, calc2)
     behavior4 = {year: {'_BE_cg': [-0.8]}}
-    behavior_y.update_behavior(behavior4)
-    calc_y_behavior4 = Behavior.response(calc_x, calc_y)
+    behv4 = Behavior()
+    behv4.update_behavior(behavior4)
+    calc2 = Calculator(policy=pol, records=rec, behavior=behv4)
+    assert calc2.behavior.has_response() is True
+    calc2_behv4 = Behavior.response(calc1, calc2)
     behavior5 = {year: {'_BE_charity': [[-0.5, -0.5, -0.5]]}}
-    behavior_y.update_behavior(behavior5)
-    calc_y_behavior5 = Behavior.response(calc_x, calc_y)
+    behv5 = Behavior()
+    behv5.update_behavior(behavior5)
+    calc2 = Calculator(policy=pol, records=rec, behavior=behv5)
+    assert calc2.behavior.has_response() is True
+    calc2_behv5 = Behavior.response(calc1, calc2)
     # check that total income tax liability differs across the
     # six sets of behavioral-response elasticities
-    assert (calc_y_behavior0.records.iitax.sum() !=
-            calc_y_behavior1.records.iitax.sum() !=
-            calc_y_behavior2.records.iitax.sum() !=
-            calc_y_behavior3.records.iitax.sum() !=
-            calc_y_behavior4.records.iitax.sum() !=
-            calc_y_behavior5.records.iitax.sum())
-    # test incorrect _mtr_xy() usage
-    with pytest.raises(ValueError):
-        Behavior._mtr_xy(calc_x, calc_y, mtr_of='e00200p', tax_type='?')
+    assert (calc2_behv0.records.iitax.sum() !=
+            calc2_behv1.records.iitax.sum() !=
+            calc2_behv2.records.iitax.sum() !=
+            calc2_behv3.records.iitax.sum() !=
+            calc2_behv4.records.iitax.sum() !=
+            calc2_behv5.records.iitax.sum())
 
 
 def test_correct_update_behavior():
     beh = Behavior(start_year=2013)
     beh.update_behavior({2014: {'_BE_sub': [0.5]},
                          2015: {'_BE_cg': [-1.2]},
-                         2016: {'_BE_charity':
-                         [[-0.5, -0.5, -0.5]]}})
+                         2016: {'_BE_charity': [[-0.5, -0.5, -0.5]]}})
     should_be = np.full((Behavior.DEFAULT_NUM_YEARS,), 0.5)
     should_be[0] = 0.0
     assert np.allclose(beh._BE_sub, should_be, rtol=0.0)

taxcalc/tests/test_utils.py

@@ -732,6 +732,9 @@ def test_multiyear_diagnostic_table(cps_subsample):
     adt = multiyear_diagnostic_table(calc, 3)
     assert isinstance(adt, pd.DataFrame)
     behv.update_behavior({2013: {'_BE_sub': [0.3]}})
+    calc = Calculator(policy=Policy(),
+                      records=Records.cps_constructor(data=cps_subsample),
+                      behavior=behv)
     assert calc.behavior.has_response()
     adt = multiyear_diagnostic_table(calc, 3)
     assert isinstance(adt, pd.DataFrame)