Skip to content

small bug fix for large mnl simulation re: specification of interaction terms - #109

Open
mxndrwgrdnr wants to merge 2 commits into
devfrom
large_mnl_intx_term_fix
Open

small bug fix for large mnl simulation re: specification of interaction terms#109
mxndrwgrdnr wants to merge 2 commits into
devfrom
large_mnl_intx_term_fix

Conversation

@mxndrwgrdnr

Copy link
Copy Markdown
Contributor

The original code I wrote to accommodate a list of tables of interaction terms breaks if the interaction terms aren't a list of dataframes but rather a single dataframe, which we don't want. This PR simply fixes the large mnl run() method to account for the latter case.

@mxndrwgrdnr

Copy link
Copy Markdown
Contributor Author

Not sure if this fix elicits its own version increase, perhaps could just get included in the next release if there's one in the works?

@coveralls

coveralls commented Jun 17, 2019

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 91.928% when pulling aa2e9ed on large_mnl_intx_term_fix into 3324905 on master.

@smmaurer

Copy link
Copy Markdown
Member

Great! It looks like the Travis failures are unrelated to your changes, but would you mind adding a commit here as described in issue #110 to see if it fixes the builds?

I'll be getting back to urbansim_templates work in July.. Happy to make a point release for this in the meantime if it would make your workflow easier, or if you think other folks might run into the same bug.

@smmaurer

Copy link
Copy Markdown
Member

Oh, right, I guess we're already into the 0.2 dev series of version numbers. You could increment this and call it 0.2.dev7. And probably no release on pip/conda until 0.2 is finished, unless there's a good reason.

@mxndrwgrdnr

Copy link
Copy Markdown
Contributor Author

Just triggered the builds with the requirements.txt fix. We'll see how it goes. I don't personally need the version update/point release, and I don't think anyone else will run into this issue. I just wanted to make sure the bug and the fix was documented for the next release. Is there an easy way to just let this PR camp out until the next release?

@smmaurer

Copy link
Copy Markdown
Member

Sweet, that worked! Fine by me to leave this PR unmerged -- i'll make sure it gets included in the next release.

@mxndrwgrdnr

Copy link
Copy Markdown
Contributor Author

👍

@smmaurer
smmaurer changed the base branch from master to dev February 16, 2021 23:51
@bouzaghrane

Copy link
Copy Markdown

@smmaurer any updates on this? can we schedule some time to chat about it?

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

Successfully merging this pull request may close these issues.

4 participants