Closed akselx closed 5 months ago
This is exciting stuff! I commented in the files but some overarching comments & questions below:
script/exogenous
folder does seem like a good place to memorialize unused items.run_setup.yaml
and doing all editing upfront makes things much more transparent and we can then just share those inputs easily. Also cuts down on code complexity. I know this is a larger conversation with @drewlevitt and his work.Hi @akselx - just taking a look before our 10am to see how it's going. I think the code that's not being used by the strategy hasn't been pulled to a sub-folder yet (additional elcm configs, model variables, etc.), or just let me know if you have different thoughts on that too.
I think the same applies to the pipeline filtering code as well, if we want to move to making pipeline edits in the inputs rather than at run time as an overall direction of the code.
Happy to discuss these in a meeting though too for other opinions if you'd like!
Did we figure out if the metrics code is okay to be here?
For the sqft_per_job variable code block- what were your thoughts though on not having the code be dependent on the presence of years within the input, but rather just selecting a set of adjusters to use based on the file name in run_setup, which is related to what we want to do in that run (calibration adjuster, telecommute strategy adjuster, external force, etc.). Smaller question was about removing the copying of the variable at the beginning and just setting it with the logic.
Thinking of ways to simplify reading the code since its 125 or so lines right now.
Did we figure out if the metrics code is okay to be here?
From all appearances, what shows up as green and "new" is already in main.
For the sqft_per_job variable code block- what were your thoughts though on not having the code be dependent on the presence of years within the input, but rather just selecting a set of adjusters to use based on the file name in run_setup, which is related to what we want to do in that run (calibration adjuster, telecommute strategy adjuster, external force, etc.). Smaller question was about removing the copying of the variable at the beginning and just setting it with the logic.
Thinking of ways to simplify reading the code since its 125 or so lines right now.
I think you are saying it is preferable to just require the years, or not, as a matter of course, and just have the model fail if a year is not present in the adjuster file?
A few notes on where we're at:
For the adjusters logic question, I think yes if it helps to make the logic more direct and is covered by the input specification / expected input columns already. Same for the small item of removing the .copy() line and using the one variable name in the function. But they're not requirements to run or anything; it seems like we may want to merge instead of putting time into that!
A few notes on where we're at:
* We're keeping the additional accessibility variables- sounds good. * We're keeping the pipeline filtering functionality- maybe not something we want to be doing at run time long term, but it's not being used currently in any case. * We've removed the optional run setup argument for housing element- I'm realizing we chose yesterday to maintain optional run setup arguments for pipeline additions (if they can be commented out, deleted, etc. in a given run setup file). In that case, it seems like the same thing could apply to the sqft per job adjuster files as well, where we use default files if certain files aren't specified. I like the explicit options but if folks prefer this we can and should do it across the board! * We're not using an alternative elcm specification, but looks like we'll keep that functionality in the code too.
For the adjusters logic question, I think yes if it helps to make the logic more direct and is covered by the input specification / expected input columns already. Same for the small item of removing the .copy() line and using the one variable name in the function. But they're not requirements to run or anything; it seems like we may want to merge instead of putting time into that!
.copy()
line and just added a return value from each if blocksqft_per_job_telecommute_file
, sqft_per_job_adj_file
and exog_sqft_per_job_adj_file.
This makes it clearer when needing the standard adjusters which file to use.
This PR is focused on:
It leverages research on space per worker trends nationally and locally to create updated adjuster files. The adjuster files are already used by the model system to apply cross-sectional detail to the regional space per worker constants.
The PR updates the existing function
sqft_per_job
invariables.py
in the following ways:Other changes