-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
update anova type II and III #391
Conversation
Thanks @clarkliming , can you add a new issue for this production work? (ideally also with checkboxes for the objectives of that issue) The other issue was only for the design I think |
Code Coverage Summary
Diff against main
Results for commit: af1155e Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Test Performance Difference
Additional test case details
Results for commit c4d26f2 ♻️ This comment has been updated with latest results. |
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.
Thanks @clarkliming , looks great!
Few suggestions:
- Could we use a similar mechanism as for
emmeans
so that we don't need to importcar
but just suggest it? - could we add a
NEWS
item - For the vignette I remember we somewhere have something on hypothesis testing already, would it make sense to integrate those together somehow?
- please add an example in introduction vignette
Updated. However added a new function (copy from emmeans) but it is difficult to do any testing on it
Already had an entry under "new features". do we need more?
Is it the section in introduction? Maybe it is clearer to be separated here because it involves a lot of details?
Added now |
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.
Thank you @clarkliming ! Great stuff. I gave it a thorough last round of review.
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
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.
Congrats @clarkliming , good to merge after last few suggestions are in!
@@ -1,3 +1,4 @@ | |||
ARMCD |
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.
maybe set in backticks to avoid the need for this?
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.
it is in the matrix part so adding backtick is ugly and really not necessary
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
close #392