-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
ENH: Improved performance of the ConditionalMNLogit class #9036
base: main
Are you sure you want to change the base?
Conversation
…p with numpy vector operations. In 'score' method, the score of the variable 'v' is calculated at a time for the selected values (exp_sum). The number 'p' is used to access them.
@josef-pkt @bashtage could you tell me why this is happening to me: statsmodels.statsmodels - #20231022.1 failed? logs:
I tried making changes to the installed library locally and running the test_conditional tests and they passed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have recently been using these models. In general this looks good to me, if you could change to logsumexp that would be helpful.
denom = np.sum( | ||
np.exp( | ||
np.sum(x[jj, list(itertools.permutations(y))], axis=1) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could remove the outer sum and exp and use scipy.special.logsumexp below for a little better stability.
denom = np.sum(x[jj, list(itertools.permutations(y))], axis=1)
ll += x[(jj, y)].sum() - logsumexp(denom)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jseabold updated the code. I posted the code for the tests in a separate message.
Updated for better stability(sum and exp replaced with scipy.special.logsumexp).
Hello @quant12345! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-05-14 13:50:47 UTC |
Below is the test_conditional_new code. The conditional_models_new code is not posted due to line limitations, I am attaching it as an attachment(with using scipy.special.logsumexp). test_conditional_new - code
The performance is the same as in my previous version. |
ENH - ConditionalMNLogit class: in
loglike
method replaces the nested loop to numpy vector operations. Inscore
method, variablev
is calculated at a time for the selected values (exp_sum). The number 'p' is used to access them.As a result,
test_conditional_mnlogit_grad
is calculated 2.7 times faster, andtest_conditional_mnlogit_2d
,test_conditional_mnlogit_3d
1.7 times faster.Below is a description of how to compare the old and new options.
I separated
test_conditional_
,conditional_models_
into separate files so that I could run the old and new options locally together for comparison (located them in a common directory). In conditional_models there are two classesConditionalMNLogit
andConditionalMNLogit_new
- in which I made improvements. In tests, the module is connected directly to use both options for tests.n = 90
is passedto gen_mnlogit(n)
Below is the
test_conditional_
code. Theconditional_models_
code is not posted due to line limitations, I am attaching it as an attachment.test_conditional_ - code
conditional_models_.py.tar.gz
Output:
**Notes**
needed for doc changes.
then show that it is fixed with the new code.
verify you changes are well formatted by running
flake8
is installed. This command is also available on Windowsusing the Windows System for Linux once
flake8
is installed in thelocal Linux environment. While passing this test is not required, it is good practice and it help
improve code quality in
statsmodels
.