Change target_units depending on type. Issue #70

UDST / developer

Redesigned UrbanSim developer/pro forma models

https://udst.github.io/developer/

BSD 3-Clause "New" or "Revised" License

3 stars 5 forks source link

Change target_units depending on type. Issue #70 #71

Closed alephcero closed 4 years ago

alephcero commented 4 years ago

Function _select_buildings() fails when self.target_units is a pd.DataFrame. While in the first lines checks for the target_units type, then it gets used in followings functions disregarding it. This happens when vacancy rates table from the UI is used in a simulation.

A new target_units object is created within the already existent type check and this gets used downstream.

smmaurer commented 4 years ago

I'm not an expert on this code, but it looks to me like it was indeed a bug, and that this would fix it. (But i defer to others who are more familiar with it!)

A couple other comments:

There's no docstring in the Developer() constructor for the target_units parameter. Would be great to add one, describing the exact requirements. For example, if it's a DataFrame, it's required to have a column also named target_units? But from the code it's not clear to me what the rows would represent.

https://github.com/UDST/developer/blob/62cd3e0f30b6ada2e91bb0d29d51c4e7f6472a8c/developer/develop.py#L31
Looks like the tests have been failing for a while (not just because of this PR), so it would be great to get those working too. And if convenient, it would be nice to have a new test confirming that both configurations of target_units are working now.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 51

3 of 5 (60.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 95.175%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
developer/develop.py	3	5	60.0%
<!--	Total:	3	5	60.0%	-->

Files with Coverage Reduction	New Missed Lines	%
developer/develop.py	1	89.12%
<!--	Total:	1		-->

Totals
Change from base Build 43:	-0.1%
Covered Lines:	789
Relevant Lines:	829

💛 - Coveralls

hanase commented 4 years ago

This code change broke our code. Our custom_selection_func needs the whole data frame (self.target_units), not just the sum (target_units): https://github.com/UDST/developer/pull/71/files#diff-eb7c3ea6968ab90a17a3b93b8876002bL443 Can we revert this one line to build_idx = custom_selection_func(self, df, p, self.target_units)

smmaurer commented 4 years ago

Hi @hanase, thanks for catching this. The new PR #73 should fix it -- does that look good?

hanase commented 4 years ago

Yes - great, thanks a lot!

cvanoli commented 4 years ago

Great! Thanks for your confirmation @hanase. I'll merge these changes now