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

test_load_data.py failing due to refactored _populate_zone_facts #594

Open
NealHumphrey opened this issue Oct 1, 2017 · 1 comment
Open

Comments

@NealHumphrey
Copy link
Collaborator

From @pfjel7:
The gerry-rigged method I used for calculating residential units for the zone_facts table has the potential to disrupt the normal functioning of test_load_data.py. When that code runs self.loader._summarize_observations (line 2328), it does not include a value for "res_units_by_zone_type" that I rewrote that function now to need. Similarly, when that code runs self.loader._populate_zone_facts_table (line 2121), it does not provide a value for that same variable. This could be a major problem for that testing function.

Is this something for which you would like me now to develop and propose a fix? I know eventually (post-launch) I intend to refactor that code, in order not to need (hopefully) that additionally passed variable. However, if this problem has the potential to throw things off in the mean time, then that probably is something I should address sooner rather than later.

@NealHumphrey
Copy link
Collaborator Author

@jkwening for background, the code you moved over from the API into load_data has that kind of funky approach to calculating zone-based weightings either when creating a rate (e.g. crime rate) or just applying weighting factors (ACS data). The funky approach was because when the function was in the API the user was able to pass in dynamic choices for things like duration and rate vs. count.

We wanted to add the count of residential units to this calculation so that building permits could also be a rate. In this PR (#556) Per changed the signature of the _populate_zone_facts_table method to pass this in. This breaks the unit tests you wrote.

As I noted in my review of #556 I think the best approach is actually to refactor this whole section to be cleaner based on how it is now used in the LoadData module. But I merged anyways so we could have this available for launch. My recommendation would be to let this test fail for now and do a full refactor, rather than editing the test to match the current signature. What do you think @jkwening?

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

No branches or pull requests

2 participants