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.

Sign up for GitHub

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

Jump to bottom

BUG fit alpha mle param #107

Merged
merged 5 commits into from
Mar 30, 2023
Merged

BUG fit alpha mle param #107

merged 5 commits into from
Mar 30, 2023

Conversation

BorisMuzellec
Copy link
Collaborator

@BorisMuzellec BorisMuzellec commented Mar 30, 2023 •
edited
Loading

Reference Issue or PRs

#106

What does your PR implement? Be specific.

This PR corrects and improves fit_alpha_mle. As pointed out by @mikelove, there were some inconsistencies between the alpha and log(alpha) parameterizations in the optimization routine.

fit_alpha_mle was also refactored to avoid unnecessarilly recomputing log(alpha) and log(alpha_hat), or computing W when cr_reg=False.

@BorisMuzellec BorisMuzellec mentioned this pull request Mar 30, 2023
Closed
@BorisMuzellec BorisMuzellec marked this pull request as ready for review March 30, 2023 12:35
@BorisMuzellec BorisMuzellec requested a review from a user March 30, 2023 12:35
maikia
maikia approved these changes Mar 30, 2023
View reviewed changes
Copy link
Collaborator

@maikia maikia left a comment

Choose a reason for hiding this comment

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

LGTM, also clearer to read now. Does it solve #106 ?

@mikelove
Copy link
Contributor

Re: 106 -- it may or may not, I can check later today.

@mikelove
Copy link
Contributor

On my end this appears to fix the MAP convergence issue 👍

Screenshot 2023-03-30 at 11 36 10 AM

@BorisMuzellec
Copy link
Collaborator Author

Awesome! Thanks a lot @mikelove for identifying the issue and checking that it was fixed, and @maikia for reviewing the PR.

@BorisMuzellec BorisMuzellec merged commit a8f62a8 into main Mar 30, 2023
@BorisMuzellec BorisMuzellec deleted the fit_alpha_mle_param branch March 30, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maikia maikia maikia approved these changes

@arthurPignetOwkin arthurPignetOwkin Awaiting requested review from arthurPignetOwkin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BorisMuzellec @mikelove @maikia