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

Add more Calculator methods and use them in Tax-Calculator code #1791

Merged
merged 19 commits into from
Jan 13, 2018
Merged

Add more Calculator methods and use them in Tax-Calculator code #1791

merged 19 commits into from
Jan 13, 2018

Conversation

martinholmer
Copy link
Collaborator

@martinholmer martinholmer commented Dec 22, 2017

This pull request continues the process of finishing the object-oriented-programming (OOP) design of Tax-Calculator described in issue #1720. Several new Calculator methods were added and used in the code so that direct access of the Records object embedded in each Calculator object can now be avoided.

Also, the Records object embedded in a Calculator object has been renamed from records to __records to make it private to the Calculator class. If you are using Tax-Calculator and directly accessing the Records object embedded in a Calculator object, then you need to use the new Calculator-class access methods instead. If there is some kind of manipulation of the embedded Records object that you cannot do easily with the new access methods, then raise an issue and we will add a new access method to facilitate your manipulations.

In addition, the Consumption object embedded in a Calculator object has been renamed from consumption to __consumption to make it private to the Calculator class.

And finally, the Behavior object embedded in a Calculator object has been renamed from behavior to __behavior to make it private to the Calculator class.

This pull request does not change tax-calculating logic or tax results.

Use of the new access methods (rather than direct manipulation of the embedded objects) has little effect on the execution time of typical Tax-Calculator tasks. For example, a script that generates MTRs for each filing unit in the cps.csv input file for each of the 18 valid MTR variables, takes 49.63 seconds (and average of three runs) to execute under taxcalc package 0.14.0 (before any of the access methods were used in the mtr method) and takes 49.75 seconds using a package built from this pull request. The increase of 0.12 seconds is about a 0.2 percent increase in run time.

A discussion of how to transition to using these new Calculator class access methods can be found here.

"""
return self.records.array_length

def getrecords(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinholmer out of curiosity, why aren't you using an underscore in the method names getrecords and setrecords? To me, the pattern indicates that this name should be get_records just like we have set_records_current_year below?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I see that you used getarray and setarray. These names make more since now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be either way, @hdoupe, but I'm leaning toward short method names to ease the transition away from directly accessing a Records object embedded in a Calculator object.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both ways make sense to me. I lean towards using the underscore. This falls more into line with the Python variable naming convention. Plus, I don't think adding an extra character costs very much.

@codecov-io
Copy link

codecov-io commented Dec 22, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1791   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        3064    3098   +34     
======================================
+ Hits         3064    3098   +34
Impacted Files Coverage Δ
taxcalc/calculate.py 100% <100%> (ø) ⬆️
taxcalc/taxcalcio.py 100% <100%> (ø) ⬆️
taxcalc/behavior.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 9a62b66...96116cd. Read the comment docs.

@martinholmer martinholmer added ready and removed WIP labels Dec 28, 2017
@martinholmer
Copy link
Collaborator Author

After pull request #1791 is merged and included in the taxcalc 0.15.0 package, applications using the taxcalc package will no longer be able to directly access the four objects embedded in a Calculator object: the Records object, the Policy object, the Consumption object, and the Behavior object. Variables in these embedded objects may be accessed using new Calculator class access methods, about which more will be said below.

If you find that your application needs an access method that has not been added, please raise a Tax-Calculator issue to explain your application's needs.

Most applications can make the transition to using taxcalc 0.15.0+ by making just a few changes. The most common direct manipulation of objects embedded in a Calculator object is getting a numpy array out of the embedded Records object or setting the value of a numpy array in the embedded Records object. Here is how to make the transition to using the taxcalc 0.15.0+ packages.

OLD USAGE                            NEW USAGE
x = calc.records.e00200p             x = calc.array('e00200p')
calc.records.e00200p = y             calc.array('e00200p', y)

Read the new calculate.py file in pull request #1791 for other new access methods. The code in calculate.py and the unit tests makes use of all these new Calculator class access methods.

@martinholmer martinholmer changed the title Add more Calculator methods and us them in Tax-Calculator code Add more Calculator methods and use them in Tax-Calculator code Jan 2, 2018
@martinholmer
Copy link
Collaborator Author

Unless there are concerns or objections, I will merge #1791 at the end of the work day on Friday, Jan 12th.

@martinholmer martinholmer merged commit ea0eeb9 into PSLmodels:master Jan 13, 2018
@martinholmer martinholmer deleted the oop-calc-records branch January 13, 2018 00:55
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.

3 participants