bayarea code review

[fscottfoti]

bayarea code review

[jiffyclub]

To your last question, add_table is the way to do that. It's designed to work seamlessly, so if it's not there's a bug.

[jiffyclub]

Using -1 to mark unfilled IDs (as opposed to real-numbered data) is fine. Are there any changes necessary in urbansim to accommodate that?

[jiffyclub]

Looks like maybe the neighborhood_vars and price_vars models could actually be table sources?

[jiffyclub]

This all seems fine! I like the way you're doing a lot of user feedback in the utils/models. User feedback is something that's hard to get right in the library code itself because you never know which users are going to want lots of feedback and which ones will find it annoying. So doing that over here, where users have control over it, is great.

One thing to watch out for is a proliferation of places that modify the simulation state. The ability to update the simulation from anywhere is convenient, but can also lead situations where something is changing the state unexpectedly and it's difficult to track down what exactly is doing it. If we ever move any of the utilities into urbansim we'll want to make them return their results rather than updating the sim themselves.

[fscottfoti]

No changes (within urbansim) necessary to use -1's to mark relocating households. Just making a comment as to why I went back to that...

neighborhood_vars and price_vars would get recomputed yearly. So they would be @sim.table rather than @sim.table_source? Is that right (either way that's a good idea)? And I'm realizing something like get_vacant_units should probably be @sim.variable instead of being in utils.

Other comments are good. I'll make a few small revisions when I get a chance. I realized that since these things are all registering functions or objects they can be registered in a random order. The real power of that is that a novice user only needs to thing of one function at a time instead of all the interconnections, which is great. Which then made me think that you could create an awesome web interface where you can browse the tables, variables, models, etc and only see one function at a time (which are usually just a few lines of code a-la a cell in the notebook). And if you're trying to set the injectables you can get a list of the currently registered injectables, and maybe even each function can be tested as you work on it. Anyway, for the distant future, but I'm really liking the flexibility.

[jiffyclub]

If you want something to be run yearly a model is the easiest way to do that. Another way is to use a table, turn on the cache, and clear the results yearly. For neighborhood_vars and price_vars is something writing those files out yearly so they have to be read back in?

Another suggestion is not skimp on comments, both docstrings and inline comments. Like any code you're going to come back to this in future and wonder why you did something one way as opposed to another, and other people are going to be reading/modifying the code too. For your web UI idea the docstrings could be especially useful for providing descriptions of the models, tables, columns, etc. in the UI.

[jiffyclub]

One thing that occurred to me today is that models themselves could be injectables.

@sim.injectable('rsh_hedonic')
def rsh_hedonic():
    return models.RegressionModel.from_yaml('rsh.yaml')

@sim.model('rsh-estimate')
def rsh_estimate(rsh_hedonic, homesales, nodes):
    # joins or whatever
    rsh_hedonic.fit(df)
    rsh_hedonic.to_yaml('rsh.yaml')

@sim.model('rsh-simulate')
def rsh_simulate(rsh_hedonic, buildings, nodes):
    # joins or whatever
    new_stuff = rsh_hedonic.predict(df)
    buildings.update_col_from_series('residential_sales_price', new_stuff)

I haven't totally thought through how much code this would save you or how much more readable it would make the simulation, but it does seem like it would make the model instances a more explicit part of the simulation and potentially move some of the yaml specificity up out of the model classes.