easystats / effectsize

:dragon: Compute and work with indices of effect size and standardized parameters
https://easystats.github.io/effectsize/
Other
338 stars 24 forks source link

Adding power vignette plus a few other changes #605

Closed pdwaggoner closed 11 months ago

pdwaggoner commented 1 year ago

Overrides previous PR #604 to include other changes. Sorry for the confusion!

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (37b48bd) 90.74% compared to head (e7049b6) 90.55%.

:exclamation: Current head e7049b6 differs from pull request most recent head eb0af9f. Consider uploading reports for the commit eb0af9f to get more accurate results

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pdwaggoner commented 1 year ago

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!

pdwaggoner commented 1 year ago

@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!

IndrajeetPatil commented 1 year ago

@pdwaggoner R CMD check should be fixed now. You can take care of the lints.

pdwaggoner commented 1 year ago

@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!

pdwaggoner commented 1 year ago

ping @DominiqueMakowski ? Not sure who is best to review here

DominiqueMakowski commented 1 year ago

@mattansb will be away till (at least) November 👶 maybe @bwiernik can help have a look :)

bwiernik commented 1 year ago

I will take a look in the next week or so. Please feel free to ping me if I'm slow 😀

pdwaggoner commented 1 year ago

Excellent, thanks @bwiernik! Latest round of checks is running now, but everything should be fixed and ready for your review whenever you're ready.

pdwaggoner commented 1 year ago

Hi @bwiernik - just a friendly ping to review and merge, per your note. Let me know if you need anything else from me. Thanks

mattansb commented 1 year ago

Hey @pdwaggoner I had a couple of minutes to look at the code part (not the explanations), and I've made some suggestions.

Overall, looks good - we can later expand with chisq.test()s etc.

Don't forget to add the vignette to _pkgdown.yml.

pdwaggoner commented 1 year ago

Excellent, thanks @mattansb ! Just added to _pkgdown.yml and requested your review on that PR (#613). Let me know what's next and/or feel free to merge both!

pdwaggoner commented 1 year ago

@bwiernik or @mattansb, any thoughts here? Should be ready to merge. Let me know if anything else is needed from me. Thanks!

DominiqueMakowski commented 1 year ago

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)

pdwaggoner commented 1 year ago

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.

bwiernik commented 1 year ago

Reading it over this morning. Thanks for your patience.

pdwaggoner commented 1 year ago

Absolutely and no worries. Not trying to bother but rather keep it from falling through the cracks. Thanks for any reviews/thoughts.

mattansb commented 1 year ago

Hey @pdwaggoner I still have a few open notes on my review - once you make those changes, I think this can be merged.

pdwaggoner commented 1 year ago

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

mattansb commented 1 year ago

Sure, just tagged you in them.

pdwaggoner commented 1 year ago

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.

mattansb commented 1 year ago

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?

pdwaggoner commented 1 year ago

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.

mattansb commented 1 year ago

Okay, I think I messed up 😅 Do you see them now?

pdwaggoner commented 1 year ago

Ah there they are! I will make those changes right now. Thanks for that. Stand by for more

pdwaggoner commented 1 year ago

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!

pdwaggoner commented 11 months ago

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!

mattansb commented 11 months ago

I made some changes (the use of pwr.t.test() in this example was not appropriate for the data at used in the example).

Once the tests pass, I'll merge.

Thanks @pdwaggoner !

pdwaggoner commented 11 months ago

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!

mattansb commented 11 months ago

Thanks again @pdwaggoner!

pdwaggoner commented 11 months ago

Absolutely! Thanks for the review and work @mattansb !