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

large mnl simulation fails if choosers is a list #68

Closed
mxndrwgrdnr opened this issue Dec 6, 2018 · 4 comments
Closed

large mnl simulation fails if choosers is a list #68

mxndrwgrdnr opened this issue Dec 6, 2018 · 4 comments
Assignees

Comments

@mxndrwgrdnr
Copy link
Member

According to the definition of large_mnl, both choosers and out_choosers can be either string type or list of string type, seen here and here, respectively. This all works fine until you get to simulation which uses orca.get_table() function on choosers/out_choosers to perform the column update and save results, seen here. I'm pretty sure this could be fixed by replacing orca.get_table() with a self._get_df() call like is done elsewhere in this model class, but perhaps it would be better to create an additional class attribute model.out_table to refer specifically to the output table that will store the results, and which can't be a list of tables, and would therefore be distinct from either model.choosers or model.out_choosers. Any thoughts?

@mxndrwgrdnr
Copy link
Member Author

Actually I think the second idea, using a new argument out_table is the only one that will work because you cannot call update_col_from_series on the output of _get_df().

@smmaurer
Copy link
Member

smmaurer commented Dec 6, 2018

Oh, good catch. I think the reasoning we've used in other places is that the first table in a list needs to be the "primary" table, i.e. the one whose rows represent the unit of analysis -- otherwise it's impossible to know how to merge the tables together properly. And in this case, we can assume/require that the output variable goes into the first table.

See here in the OLS docstrings for example. So i guess we need to replace MNL#L524-L527 with something like TemplateStep#L290-L308 that i wrote for OLS..

@mxndrwgrdnr
Copy link
Member Author

Ah, yes, that looks like a more elegant solution. I'll submit the PR.

@mxndrwgrdnr
Copy link
Member Author

Fixed in #69.

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

No branches or pull requests

2 participants