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.

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

Add tests to test_emuFit_micro comparing closed-form MPLEs to MPLE obtained via numerical optimization #95

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

ailurophilia
Copy link
Collaborator

…tained via numerical optimization

@adw96 adw96 requested review from svteichman and gthopkins October 24, 2024 11:22
@adw96
Copy link
Contributor

adw96 commented Oct 24, 2024

@svteichman Do you have time to review? I thought this might be of interest since you've been in the weeds of the optimization. If not, @gthopkins is qualified and capable! Your call

@adw96
Copy link
Contributor

adw96 commented Oct 24, 2024

@gthopkins let's save @svteichman 's time for her defense -- can I please ask you to review this? You don't need to review the algebra, just that the test's purpose is clear, well-commented, reasonable, etc. I think this should be straightforward 😸

@adw96 adw96 removed the request for review from svteichman October 24, 2024 16:59
@gthopkins gthopkins merged commit 91336cf into statdivlab:main Oct 24, 2024
4 checks passed
@gthopkins
Copy link
Collaborator

No problem @adw96! I just merged this PR--it all makes sense and it organized well. The only tiny note I have is that the titles of the test_that() functions are not symmetrical:

  1. PL fit with categorical predictor matches analytical form of MPLE in this case, and does NOT match MLE
  2. PL fit with categorical predictor matches analytical form of MPLE in this case, and does NOT match MLE when group sizes are unequal

For clarity, it would generally make sense to add "when group sizes are equal" to the first title, however this is more of a linguistic note than anything. I figured I would not hold up the PR for such a small detail :)

@adw96
Copy link
Contributor

adw96 commented Oct 27, 2024

Thanks so much, @gthopkins!

Quick question -- did you make the changes in line with your linguistic note, or did you just accept the pull request as is? Pull requests aren't just a take-it-or-leave-it -- you can commit to them to make them better. If you didn't make the changes to make this better, would you mind making them now and commiting to main? (a mini PR) I think it's important that we make this repo as good as possible, especially if there are things that are easy to do that add clarity for future maintainers.

Thank you so much!!!

gthopkins added a commit to gthopkins/radEmu that referenced this pull request Oct 27, 2024
@gthopkins
Copy link
Collaborator

Thank you for this reminder, @adw96! I have included this small linguistic edit in PR #92, so it will be changed along with our use of simulate_data in the testing files.

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