taxdata icon indicating copy to clipboard operation
taxdata copied to clipboard

Refactor solve_lp_for_year.py

Open MaxGhenis opened this issue 6 years ago • 2 comments

Not ready to merge

In filing #323 I looked through this file, and took a stab at streamlining the code. Changes include:

  1. Using a wage_bin function to create wage bins.
  2. Using a loop to assign lhs_vars dictionary elements.*
  3. Using a loop to assign factor variables.
  4. Using an add_target function to assign rhs_vars dictionary elements.*
  5. More explicitly adjust model_vars for year 2014.
  6. Rename LP to lp since it's not a constant.
  7. Other minor changes like comments.

* These pieces won't currently work since they attempt to access globals() from within a function (locals() also won't work). Per https://stackoverflow.com/q/56983782/1840471, there's no good way to access these in-between-scoped variables, and this is really a sign that something could be done better. It seems to me that sticking with the dictionary elements rather than creating new variables would be a clean alternative.

I haven't tested this code yet. Would the right way be to run make cps-files?

MaxGhenis avatar Jul 11 '19 18:07 MaxGhenis

Thanks for working on this, @MaxGhenis! These changes look good so far.

I haven't tested this code yet. Would the right way be to run make cps-files?

Yes. Once you run this it'll create a new weights file and you can see if the results have changed at all. Fair warning, this will take a few hours to run.

andersonfrailey avatar Jul 15 '19 13:07 andersonfrailey

Sorry I've left this open so long. I'll resolve conflicts, boil it down to the basics, and test it, once #343 is merged.

MaxGhenis avatar Jul 19 '20 23:07 MaxGhenis