search

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

Settings parcel_sizes should not influence the profit #67

Open hanase opened 5 years ago

hanase commented 5 years ago

The feasibility model expects users to set parcel_sizes in the yaml file (default is 10000). A comment in the code claims that the value does not matter because it cancels out in the pro-forma computation. This is true for some of the pro-forma measures. However, this settings influences the construction time and hence the profit. Here is more detail.

Probably an easy fix would be to keep the construction time on the per-sqft scale and then multiply it by the real parcel size in _lookup_parking_cfg().

jessicacamacho commented 5 years ago

Hi @hanase ,

Thanks for pointing this out. We have changed _lookup_parking_cfg( ) in a branch called fixes_months_calculation, so that actual parcel sizes are taken into account when calculating construction months.

@hanase, @janowicz: please take a look at https://github.com/UDST/developer/compare/fixes_months_calculation, and let me know if you have any questions or suggestions before creating a PR.

Thanks!!

Jessica

hanase commented 5 years ago

Hi @jessicacamacho , thanks for looking into this! It looks to me like the computation of month_values is not correct. The self.construction_months object is a matrix with columns corresponding to uses and rows corresponding to months (created here), so it's not form-specific.

jessicacamacho commented 5 years ago

Hi @hanase,

Thanks for your comment. I have made a modification that should solve it, weighting the construction_months values by use (from proforma.yaml) with the percentage of each use on each form: https://github.com/UDST/developer/blob/9f3d8ebc2667b26ae97ebfd81bb36c3dee5733b3/developer/sqftproforma.py#L696-L698

Please let me know if you have any additional suggestions or questions.

Thanks!

Jessica

hanase commented 5 years ago

This works and gives the same results for different values of parcel_sizes. Thanks @jessicacamacho ! My suggestion would be to remove the need to have the parcel_sizes settings in the yaml file as it is quite confusing. Without reading the code one would not know that this setting has no impact on anything.