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

[Review] Update stage 2 to Julia #351

Merged
merged 22 commits into from Aug 21, 2020

Conversation

chusloj
Copy link
Contributor

@chusloj chusloj commented Aug 3, 2020

This PR addresses #348 . This will replace both the CPS and PUF stage2 Python files with Julia files and change README files accordingly.

@chusloj chusloj marked this pull request as draft August 3, 2020 19:41
@chusloj chusloj changed the title Update stage 2 to Julia Update stage 2 to Julia [WIP] Aug 3, 2020
@MaxGhenis
Copy link
Contributor

@chusloj FYI, I refactored some of the Python code behind this in #324 to avoid code duplication. Happy to do that after you're done, though I might ask you for some tips as I haven't worked with Julia before, or if it's easy for you to do while you're in here feel free.

Also some of the multi-line statements look like they need some extra indentation to align.

@chusloj chusloj marked this pull request as ready for review August 4, 2020 14:26
@chusloj
Copy link
Contributor Author

chusloj commented Aug 15, 2020

The code for the Julia solver update is finished. Overview:

  • All data processing in Julia has been scrapped and Julia is only being used for the LP solver
  • The solve_lp_for_year file has been split into a dataprep.py file and a solver.jl file.
  • Stage 2 now uses dataprep.py to save numpy arrays as .npz files to be used in the Julia solver code and then read back into Python.
    • Originally, Julia was being used for data processing by using Pandas.jl, but the package proved to be clunky, underdeveloped and very slow, and so now the standard pandas package is being used for data processing.
  • The stage2.py file has been refactored to remove redundant code.

Regarding the reproducibility issue, I have not yet checked on the md5 value after multiple runs of the stage 2 code.

UPDATE: @andersonfrailey and myself have confirmed that the reproducibility issue does not pop up anymore, but for the issue to be solved, make all must be run.

@andersonfrailey andersonfrailey added CPS enhancement in progress PUF extrapolation Issues/PRs related to our extrapolation techniques labels Aug 16, 2020
@andersonfrailey
Copy link
Collaborator

Just to put this in there before I forget, we need to install three Julia packages to run the new stage 2 scripts: Cbc, JuMP, NPZ. I haven't checked to see if there's a way to specify this in the enviornment.yml file. If not we'll just need to specify the need to install them in the docs.

@chusloj chusloj changed the title Update stage 2 to Julia [WIP] Update stage 2 to Julia [Review] Aug 17, 2020
@chusloj
Copy link
Contributor Author

chusloj commented Aug 18, 2020

A Julia environment now exists as a .toml file in the top-level directory. The structure is similar to .yml files used in conda, but Julia's package manager does not have an option for activating a package at the command line, so the environment is being activated when solver.jl is run using:

Julia --project={env_path} solver.jl

in stage2.py.

@chusloj chusloj changed the title Update stage 2 to Julia [Review] [Review] Update stage 2 to Julia Aug 18, 2020
@chusloj
Copy link
Contributor Author

chusloj commented Aug 18, 2020

After comparing Tax-Calculator projections from the PUF and CPS weights before and after the Julia solver was implemented, results show that projections don't change.

If this seems irregular, note that the solver Julia is using right now (Cbc) is the same solver that PuLP was using before CVXOPT and Julia/JuMP were implemented.

@andersonfrailey
Copy link
Collaborator

Thanks for working on this, @chusloj. I'm going to leave this PR open for a couple days to allow feedback from others.

@andersonfrailey
Copy link
Collaborator

If there are no objections, I'll merge this PR at COB today.

@andersonfrailey andersonfrailey merged commit 3d39ff6 into PSLmodels:master Aug 21, 2020
@chusloj chusloj deleted the julia_update branch January 26, 2021 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CPS enhancement extrapolation Issues/PRs related to our extrapolation techniques PUF review ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants