UDST / choicemodels

Python library for discrete choice modeling
https://udst.github.io/choicemodels
BSD 3-Clause "New" or "Revised" License
74 stars 33 forks source link

Benchmarks and efficiency improvements for MergedChoiceTable #11

Closed smmaurer closed 6 years ago

smmaurer commented 7 years ago

For large datasets, MergedChoiceTable is pretty slow.

Example:

Can we speed this up? A quick investigation could look at:

  1. How does the speed of MergedChoiceTable compare with the underlying mnl_interaction_dataset()?

  2. Which use cases and parts of the code cause the biggest bottlenecks?

  3. How might we speed those things up?

Eh2406 commented 7 years ago

You rang. :-P

Do you have a reproducible test case and sample data I can use to profile?

smmaurer commented 7 years ago

Good question :)

The example that inspired this issue was from one of our lab projects, but it will be easier to work with the travel survey dataset that we've been using to put together demo notebooks.

Instructions for loading the data are here: https://github.com/UDST/choicemodels/tree/master/data https://github.com/UDST/choicemodels/blob/master/notebooks/Data-prep-01.ipynb

This notebook sets up equivalent data merges using mnl_interaction_dataset() and MergedChoiceTable(), but we should probably enlarge the sample size: https://github.com/UDST/choicemodels/blob/master/notebooks/Destination-choice-models-01.ipynb

All the underlying code is in this file: https://github.com/UDST/choicemodels/blob/master/choicemodels/tools/interaction.py

Eh2406 commented 7 years ago

I did not have time to experiment with this before my vacation. Just from looking at the code without running it. MergedChoiceTable looks to be a thin wrapper around mnl_interaction_dataset. So I would be surprised if the answer to 1 is not "almost identical". But I am often surprised, that is why performance is hard.

For mnl_interaction_dataset, I think that if you run line_profile on it you'll discover most of the time is in np.random.choice. The only thing we can do to speed that up is to make the arguments smaller somehow. If another line is slow, than we should fix it.

Other thoughts, I did not understand this removing perhaps we can find a clearer way to get the same functionality?

smmaurer commented 7 years ago

My guess about those lines is that it's handling a case where invalid alternatives where included in the input data. Seems better to me to just enforce that all the inputs be valid.

In general, the code we ported over from urbansim.urbanchoice leaves something to be desired in its documentation and clarity. You're right that the new class definitions are in large part just wrapping the old functions, but the goal is to be more explicit about inputs, outputs, and behavior. That should make it easier for this to be a standalone library, and easier to refactor some of the old code without unintended consequences.

smmaurer commented 6 years ago

Code was fully refactored in PR #37. Moving discussion of performance optimization to Issue #39.