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

[0.1.dev17] latest updates to segmented MNL #61

Merged
merged 6 commits into from
Nov 16, 2018

Conversation

mxndrwgrdnr
Copy link
Member

this PR addresses:

@mxndrwgrdnr mxndrwgrdnr requested a review from smmaurer November 14, 2018 02:32
Copy link
Member

@smmaurer smmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! Some comments:

  1. Should add docstrings for LargeMultinomialLogitStep.run() explaining that chooser_batch_size will be passed through to iterative_lottery_choices(), and is a temporary solution that will eventually be replaced by a class parameter/setting (i can take care of it with issue Settings objects for groups of common parameters #54).

  2. In the same place, instead of adding an interaction_terms argument, would you want to just accept a custom MergedChoiceTable like is done higher up in the fit() method? This is more flexible, but i guess might be more work for the user. Either one would be temporary -- issue Settings objects for groups of common parameters #54 will include a solution for orca to manage interaction terms.

  3. Can you bump the version number in setup.py and the top-level __init__.py? The current master branch is 0.1.dev16, so you can make this dev17. I'll wait to merge the other open PR, and can change it to dev18.

The tests look good and all pass on my machine.

I like your temporary solution for printing the segment name. Ok if i wait to implement the full solution from issue #59? We can leave it open so i don't forget.

@mxndrwgrdnr
Copy link
Member Author

Thanks for the notes Sam. Here are my answers:

  1. Done
  2. I agree that would be more consistent with how the mct is specified in the fit() method, but a) I think that might defeat the purpose of the chooser batching since the idea there is to delay the creation of the mct's until the batches have been chunked out to avoid having to hold a giant table in memory; and b) it would require more changes downstream to choicemodels code since currently iterative_lottery_choices() requires alts and obs to be passed in separately. It would be way more straightforward to implement what you suggest if we didn't care about the constrained choice scenario, but I don't see an easy way to do it otherwise. Open to any ideas you might have here, though.
  3. Done

Also, yes, sorry, I actually didn't mean to commit the segment name printing change to the code so I'm happy to ignore it.

@smmaurer
Copy link
Member

  1. Hadn't thought of that. Let's stick with your approach!

@smmaurer
Copy link
Member

And thanks for the changes! Looks great to merge.

@mxndrwgrdnr mxndrwgrdnr merged commit 21d2acc into master Nov 16, 2018
@mxndrwgrdnr mxndrwgrdnr deleted the large_mnl_sim_w_interactions branch November 16, 2018 02:47
@smmaurer smmaurer changed the title latest updates to segmented MNL [0.1.dev17] latest updates to segmented MNL Nov 19, 2018
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.

2 participants