-
-
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
Adding power vignette plus a few other changes #605
Conversation
# Description Adding a vignette on the value of statistical power as well as the role of `effectsize` in making this an easy thing to do via easystats#599 # Proposed Changes In addition to adding new vignette (`statistical_power.Rmd`), other changes include: - adding three new refs to `bibliography.bib` - adding myself as ctb in `DESCRIPTION - fixing a few typos here and there # Question **Note**: Need to change version number? Didn't think so with only the inclusion of a new vignette, but feel free to bump if needed.
updating bib for pwaggoner's new power vignette
adding pwaggoner
small typo and grammatical fixes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #605 +/- ##
==========================================
- Coverage 90.74% 90.55% -0.19%
==========================================
Files 57 56 -1
Lines 3564 3419 -145
==========================================
- Hits 3234 3096 -138
+ Misses 330 323 -7 ☔ View full report in Codecov by Sentry. |
All of these "fails" should be easy to override as the vignette successfully renders on my end. Let me know if something needs to be changed though. Thanks! |
fixing some linting issues
Co-authored-by: Dominique Makowski <dom.mak19@gmail.com>
fixed warning issues
@IndrajeetPatil - Can you help out a little on this one? These checks keep failing, and I am not sure why. I've checked and updated obviously problematic spots. The changes I am making are pretty minimal. Any help would be appreciated! |
@pdwaggoner R CMD check should be fixed now. You can take care of the lints. |
@IndrajeetPatil - Thank you so much for the quick help! Seems the other issues are with dependencies re: other easystats packages. Not sure about those. Other minor things @mattansb should be able to address. Thanks! |
ping @DominiqueMakowski ? Not sure who is best to review here |
reverting two small changes back to fix lint issues
I will take a look in the next week or so. Please feel free to ping me if I'm slow 😀 |
Excellent, thanks @bwiernik! Latest round of checks is running now, but everything should be fixed and ready for your review whenever you're ready. |
What about we add a disclaimer on top of this vignette "this vignette is work in progress, please make us a feedback about what features would you like to see in easystats to make power analysis easier", and then we merge, and Brenton will have a look when he can (and let's not bother Mat' at the moment unfortunately he has much more important stuff to worry about) |
update per Dom's disclaimer suggestion
Great idea @DominiqueMakowski - and of course re: Mat! I hope he is safe and ok. I added the disclaimer per your suggestion in the latest edit. Feel free to merge as you're able. Thanks again. |
Reading it over this morning. Thanks for your patience. |
Absolutely and no worries. Not trying to bother but rather keep it from falling through the cracks. Thanks for any reviews/thoughts. |
Hey @pdwaggoner I still have a few open notes on my review - once you make those changes, I think this can be merged. |
Thanks @mattansb ! but I can't see anything other than your suggestion to update the yml, which I did. Did I miss something? Can you point me to where you're referencing, and I will be happy to respond accordingly. Thanks |
Sure, just tagged you in them. |
Sorry, just checked but still can't locate where your comments are. Is it possible we are looking at different version of the vignette? Feel free to share a direct link, due to my denseness. |
Weird... If I go to https://github.com/easystats/effectsize/pull/605/files in a desktop browser, I see all the comments. Does that work? |
Ok, glad to know we are doing the same thing. Yes, I did just that and don't see any comments. So sorry! Feel free to make the changes directly, or you could just list them here in the conversation, and I can make them. Your call. Thanks again for the patience and help. |
Okay, I think I messed up 😅 |
Ah there they are! I will make those changes right now. Thanks for that. Stand by for more |
made changes responding to recent code review
Alright, @mattansb , all requested revisions have been responded to. Let me know what's next or if anything else is needed from me. Thanks again and I hope you're staying safe! |
Just wondering if we can push this, given that all changes were made? Thanks @mattansb for any update on where we are with this one. Thanks! |
I made some changes (the use of Once the tests pass, I'll merge. Thanks @pdwaggoner ! |
Thanks @mattansb - looks like two tests are failing elsewhere, not in the vignette, unless I am misreading. Not sure if anything is needed from me or not on this...? Thanks! |
Thanks again @pdwaggoner! |
Absolutely! Thanks for the review and work @mattansb ! |
Overrides previous PR #604 to include other changes. Sorry for the confusion!