search

UDST / bayarea_urbansim

UrbanSim implementation for the San Francisco Bay Area
14 stars 26 forks source link

Incompatibility with UDST/urbansim PR #172 #80

Closed smmaurer closed 8 years ago

smmaurer commented 8 years ago

I think UDST/urbansim PR #172 creates a wrinkle in UDST/bayarea_urbansim that needs to be ironed out.

Previously, UDST/bayarea_urbansim ran fine for me, using the return-on-cost branch of UDST/urbansim. PR #172 moved the return-on-cost functionality into the master branch (I think), but now the UDST/bayarea_urbansim alt_feasibility() step crashes with a KeyError on 'max_far_from_dua'.

https://github.com/UDST/bayarea_urbansim/blob/master/baus/models.py#L221-L225 https://github.com/UDST/bayarea_urbansim/blob/master/configs/settings.yaml#L301-L322

I'm not that familiar with the feasibility code, but am happy to dig into it if I'm the only one affected by this. Probably we just need to change how the options are being passed into UrbanSim.

@fscottfoti, does anything jump out here?

Many thanks!

fscottfoti commented 8 years ago

Did you update urbansim_defaults too?

smmaurer commented 8 years ago

Updating urbansim_defaults didn't fix it for me, but I've been taking a look at the MTC/bayarea_urbansim fork. That version doesn't pass the 'max_far_from_dua' column into the feasibility model step anymore, and it runs fine for me.

https://github.com/MetropolitanTransportationCommission/bayarea_urbansim/blob/master/configs/settings.yaml#L520-L541

I realized that it's not necessarily the most recent urbansim PR that caused this to show up -- it could be any of the differences between the former return-on-cost branch and the current master. Here's what the diff looks like: https://github.com/ual/urbansim/compare/return-on-cost...ual:master

My interpretation is that 'max_far_from_dua' is calculated endogenously in the feasibility step now, so doesn't need to be included as a pass-through variable any more.

Would you want to push one of the more recent versions of MTC/bayarea_urbansim up to UDST? Alternatively i could just make a little patch that removes the 'max_far_from_dua' key from the feasibility settings. It would be nice to keep UDST/bayarea_urbansim running with the latest versions of urbansim and urbansim_defaults.

fscottfoti commented 8 years ago

Yes, you're right. There must have been a time when I passed the column in but it never made it into master. But you're right that the solution is to merge the latest Bay Area UrbanSim - will follow up with an email on that one.

fscottfoti commented 8 years ago

ok code has been synced

smmaurer commented 8 years ago

Thanks Fletcher! Everything working great now.