Skip to content
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

Resolve issue 304 regarding Calculator constructor logic #1582

Merged
merged 7 commits into from Oct 17, 2017
Merged

Resolve issue 304 regarding Calculator constructor logic #1582

merged 7 commits into from Oct 17, 2017

Conversation

martinholmer
Copy link
Collaborator

@martinholmer martinholmer commented Oct 9, 2017

This issue resolves issue #304 by revising the Calculator class constructor logic so that deep copies of the specified Policy and Records objects are assigned to the internal class policy and records attributes. In the same spirit as #304, the Calculator constructor now also makes a deep copy of Behavior objects and Consumption objects specified in the constructor call.

The reason for doing this is the reason highlighted in issue #304: the Calculator object should not cause side effects for the Policy or Records (or Behavior or Consumption) objects passed to the Calculator constructor.

However, there is another significant benefit from making these changes: the new idiom for creating current-law and reform Calculator objects is noticeably faster than the approach used now. The execution times cited below indicate a 35 percent reduction in execution time for a typical analysis situation.

One implication of these changes is that typical current usage of Behavior objects in Calculator objects may need to change in order to get correct behavior. One example of a needed change would be the behavioral-effect notebook. Part of this pull request is to make logically necessary changes in the use of a Behavior object in a Calculator object.

Another implication of these changes is that typical non-Behavior instantiation of current-law and reform Calculator objects is correct but inefficient (that is, does not automatically get the significant execution speed up mentioned above and described in detail below). A subsequent pull request will change the pytest unit tests to use the new more efficient Calculator constructor idiom.

Here is the script used to generate the execution times (OLD_IDIOM was set to True on the master branch and set to False on this development branch).

"""
Timings before and after resolve-issue-304 changes.
"""
import time
from taxcalc import Policy, Records, Calculator

REFORM_YEAR = 2018
REFORM = {REFORM_YEAR: {'_SS_Earnings_c': [9e99]}}

OLD_IDIOM = False

EXEC_TIMES = list()
for iteration in range(0, 4):
    start_time = time.time()

    if OLD_IDIOM:
        rec1 = Records.cps_constructor()
        pol1 = Policy()
        calc1 = Calculator(policy=pol1, records=rec1, verbose=False)
        rec2 = Records.cps_constructor()
        pol2 = Policy()
        pol2.implement_reform(REFORM)
        calc2 = Calculator(policy=pol2, records=rec2, verbose=False)
    else:
        rec = Records.cps_constructor()
        pol = Policy()
        calc1 = Calculator(policy=pol, records=rec, verbose=False)
        pol.implement_reform(REFORM)
        calc2 = Calculator(policy=pol, records=rec, verbose=False)

    calc1.advance_to_year(REFORM_YEAR)
    calc2.advance_to_year(REFORM_YEAR)

    calc1.calc_all()
    calc2.calc_all()

    taxrev1 = (calc1.records.combined * calc1.records.s006).sum() * 1e-9
    taxrev2 = (calc2.records.combined * calc2.records.s006).sum() * 1e-9

    print('iter,taxrev1,taxrev2($B)= {} {:.3f} {:.3f}'.format(
        iteration + 1, taxrev1, taxrev2))

    end_time = time.time()
    exec_time = end_time - start_time
    if iteration > 0:
        EXEC_TIMES.append(exec_time)
    print('iter,total_exec_time(secs)= {} {:.2f}'.format(
        iteration + 1, end_time - start_time))

SUM_EXEC_TIMES = 0.0
for etime in EXEC_TIMES:
    SUM_EXEC_TIMES += etime
print('OLD_IDION,average_exec_time(secs)= {} {:.2f}'.format(
    OLD_IDIOM, SUM_EXEC_TIMES/len(EXEC_TIMES)))

The results of running this script are as follows:

ON MASTER BRANCH:
$ python issue_304_timing.py
iter,taxrev1,taxrev2($B)= 1 2551.551 2682.908
iter,total_exec_time(secs)= 1 29.59
iter,taxrev1,taxrev2($B)= 2 2551.551 2682.908
iter,total_exec_time(secs)= 2 15.49
iter,taxrev1,taxrev2($B)= 3 2551.551 2682.908
iter,total_exec_time(secs)= 3 15.50
iter,taxrev1,taxrev2($B)= 4 2551.551 2682.908
iter,total_exec_time(secs)= 4 15.47
OLD_IDION,average_exec_time(secs)= True 15.49

ON DEVELOPMENT BRANCH:
$ python issue_304_timing.py
iter,taxrev1,taxrev2($B)= 1 2551.551 2682.908
iter,total_exec_time(secs)= 1 23.81
iter,taxrev1,taxrev2($B)= 2 2551.551 2682.908
iter,total_exec_time(secs)= 2 10.00
iter,taxrev1,taxrev2($B)= 3 2551.551 2682.908
iter,total_exec_time(secs)= 3 9.90
iter,taxrev1,taxrev2($B)= 4 2551.551 2682.908
iter,total_exec_time(secs)= 4 9.96
OLD_IDION,average_exec_time(secs)= False 9.95

@codecov-io
Copy link

codecov-io commented Oct 9, 2017

Codecov Report

Merging #1582 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1582   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        2743    2743           
======================================
  Hits         2743    2743
Impacted Files Coverage Δ
taxcalc/calculate.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20aee9b...572563e. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants