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

Refactor solve_lp_for_year.py #324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MaxGhenis
Copy link
Contributor

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?

*Not ready to merge*

In filing PSLmodels#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, 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`?
@andersonfrailey
Copy link
Collaborator

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.

@MaxGhenis
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants