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

replace locals() update with global dictionary #10

Closed
iliakur opened this issue Aug 26, 2014 · 2 comments
Closed

replace locals() update with global dictionary #10

iliakur opened this issue Aug 26, 2014 · 2 comments

Comments

@iliakur
Copy link
Contributor

iliakur commented Aug 26, 2014

I knew from somewhere that we really should not be modifying the locals() dictionary as it's part of Python's scaffolding, but didn't vocally object too much because a) there were more important things on our plate and b) I wasn't 100% sure I was right. I now have conclusive proof to back my opinion up and would like to make sure our code adheres to the standard.
Conceptually, it's an easy fix: we simply use the "y" (or whatever we decide to name it) dictionary with our tax parameter names as keys.

In practice this might take some time since we have to go through the entire script changing all the appropriate variable names to dictionary keys.

In case you're worried about performance issues associated with the additional step of looking up the dictionary name, then the key, I have two things to say:

  1. Variable lookup is implemented very fast in Python, as it is a fundamental feature. Even with all the hundreds of lookups we'll be introducing, the overhead will be negligeable compared to other bottlenecks.
  2. To further speed things up we can pass the name of the dictionary as an argument to our calculating functions. Since Python variable names are references to an object, we won't be shunting too much memory around. At the same time, having a reference to the dictionary in the local namespace of the function will speed up its lookup even further.
@talumbau
Copy link
Member

This idea makes sense to me too. +1.

@MattHJensen
Copy link
Contributor

Dealt with in #70 among others.

@iliakur iliakur removed their assignment Oct 20, 2015
Amy-Xu pushed a commit to Amy-Xu/Tax-Calculator that referenced this issue Mar 9, 2016
jdebacker pushed a commit that referenced this issue Dec 17, 2022
jdebacker pushed a commit that referenced this issue Jun 7, 2023
Update reform file and aggregate files
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

No branches or pull requests

3 participants