Skip to content

BUG fit alpha mle param#107

Merged
BorisMuzellec merged 5 commits intomainfrom
fit_alpha_mle_param
Mar 30, 2023
Merged

BUG fit alpha mle param#107
BorisMuzellec merged 5 commits intomainfrom
fit_alpha_mle_param

Conversation

@BorisMuzellec
Copy link
Copy Markdown
Collaborator

@BorisMuzellec BorisMuzellec commented Mar 30, 2023

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 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
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@mikelove
Copy link
Copy Markdown
Contributor

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

Screenshot 2023-03-30 at 11 36 10 AM

@BorisMuzellec
Copy link
Copy Markdown
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants