arteagac / xlogit

A Python package for GPU-accelerated estimation of mixed logit models.
https://xlogit.readthedocs.io
GNU General Public License v3.0
49 stars 22 forks source link

Add option to skip standard error estimation #19

Closed gc-axa closed 1 year ago

gc-axa commented 1 year ago

Hi @arteagac, thanks for this great package.

Please allow me to give some context on this PR. This is something we introduced in our codebase, since we can do without the estimation of standard errors. Especially since, in our case at least, the estimation takes merely ~30s, but the inversion of the hessian required for the standard errors takes hours. That is why, we thought it might be beneficial also for others to have the option to control the estimation of standard errors.

The code is a minimal change to achieve our goal, happy to discuss a more generic approach.

Cheers, gc

arteagac commented 1 year ago

Hi @gc-axa. Thanks a lot for your contribution. This seems like an interesting feature that could benefit some users. Let me take a closer look at the implementation and I will get back to you.

arteagac commented 1 year ago

@gc-axa, I checked your implementation and it looks good. I want to ask you a favor, can you please implement this same parameter in the fit function for MixedLogit (https://github.com/arteagac/xlogit/blob/master/xlogit/mixed_logit.py)? MixedLogit and MultinomialLogit are similar in terms of pre-processing and input parameters, so you can basically copy/paste the code you wrote for MultinomialLogit. Additionally, I think your IDE is deleting white spaces in blank lines, which this introduces additional changes in the commit and makes it difficult to see the actual changes you implemented. Can you please omit this deletion of spaces in blank lines?

gc-axa commented 1 year ago

Thanks for the feedback, I added the changes also to MixedLogit, I reverted the whitespace changes, sorry about that. (FYI GitHub has the option to ignore whitespace changes in the PR)

btw we are experimenting with adding a regularization term (ridge regression) to the multinomial estimator. Do you have any plans of adding it yourself? Should I open an issue to discuss it further?

arteagac commented 1 year ago

@gc-axa Thank you again for this valuable contribution and for reverting the whitespace changes. Sorry, I didn't know GitHub has the option to ignore whitespaces diffs in a PR. I merged your changes and will release a new version of xlogit to PyPi with these changes soon.

Regarding ridge regression, it sounds like an interesting implementation, but first let me read a bit regarding how it would work for Mixed Logit models. The xlogit package maintains equivalent functionalities in the Multinomial and Mixed Logit models, so if we implement it for Multinomial, I would like it implemented in Mixed Logit too. I will read about it and let you know.