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

JOSS review #2 #227

Closed
11 tasks done
jkarreth opened this issue Dec 9, 2020 · 9 comments
Closed
11 tasks done

JOSS review #2 #227

jkarreth opened this issue Dec 9, 2020 · 9 comments
Assignees
Labels
docs 📑 Improvements or additions to documentation

Comments

@jkarreth
Copy link

jkarreth commented Dec 9, 2020

@mattansb This is the second review of the JOSS submission. The first review by @trashbirdecology was already amazingly thorough, so much credit to @trashbirdecology for working through the vast majority of potential issues.

The review here is split in three parts: (1) issues I'd ask you to fix or explain, (2) issues I suggest considering before the article is accepted, and (3) thoughts for future iterations of the project.

I really like this package. I'm not an easystats user, but am very impressed by how well connected and well-thought-out the easystats ecosystem is. Fantastic effort!

Issues I'd ask you to fix or explain

  • The function call to the cars example in the JOSS paper has an extra comma. Not an error, but consider correcting for aesthetic reasons.

  • The address & publisher for Cohen (1988) seem odd. Please correct to: New York: Academic Press, Inc. (via here)

  • In the standardize_parameters vignette, I'd recommend using parameters::model_parameters, like in the other vignettes.

  • This is probably a dumb question and reveals my ignorance, but why do most functions not return a standard error? E.g. lm(Sepal.Length ~ Petal.Length, data = iris) %>% standardize_parameters
    could return a standard error in addition to CIs? It's OK if this is a choice by the package developers; I'm just thinking that many novice users would look for SEs, too.

  • Following the other review, I thought the package main branch is now called "main" instead of "master", but "master" still exists and appears ahead of "main". Is that intended? I see that issue Rename "master" branch to "main" branch #201 is still open, so you're most likely onto that?

  • In the JOSS paper's first paragraph, you might consider adding a reference to latent variables around the "(especially when units of measurement are not meaningful)". Standardized effect sizes are particularly useful when using latent factor scores, for example. Adding this might clarify the package's appeal to some additional readers.

Issues I suggest considering before the article is accepted

  • It would be nice if the convert vignette had the first example data available to insert/reproduce, i.e. create it within the vignette.

  • The package help files ould describe the value of data frames that results from function calls better. Maybe an example would help the reader quickly use this data frame for further processing. This could also help with something like combining functions, such as a hypothetical call to interpret_d(standardize_parameters(mod)$Std_Median, rules = "cohen1988").

  • the paper could also mention the scale() function to standardize a matrix. That's what I was taught in my first R course many, many years ago :)

Thoughts for future iterations of the project

  • Consider replacing examples using the iris data with another dataset, e.g. mtcars or penguins. See here for why.

  • A nice-to-have from my perspective would be if the output from effectsize could easily be passed on to other packages that allow automated output in the form of regression tables (texreg, stargazer, broom, etc.).

mattansb added a commit that referenced this issue Dec 9, 2020
mattansb added a commit that referenced this issue Dec 9, 2020
mattansb added a commit that referenced this issue Dec 9, 2020
mattansb added a commit that referenced this issue Dec 9, 2020
@mattansb mattansb self-assigned this Dec 9, 2020
@mattansb mattansb added the docs 📑 Improvements or additions to documentation label Dec 9, 2020
@mattansb mattansb mentioned this issue Dec 9, 2020
@mattansb
Copy link
Member

mattansb commented Dec 9, 2020

Hi @jkarreth !

Thanks for the kind words!

I have responded to your comments below. Let me know if I've missed anything or miss-addressed anything.
All changes are in the #228 PR.

Thanks!

Issues I'd ask you to fix or explain

  • The function call to the cars example in the JOSS paper has an extra comma. Not an error, but consider correcting for aesthetic reasons.

Fixed!
94744d8#diff-e504eb580b095a7e65428b098183a581e475f0fb316db95287eacd7d4f344424L112

  • The address & publisher for Cohen (1988) seem odd. Please correct to: New York: Academic Press, Inc. (via here)

Done (5d38a77).

Oh, good suggestion - fixed!
6075bae#diff-40521620de914f93819a37dd1d1705cdb4fc8b341fcee916f2cbd02ca5a128ffR67

  • This is probably a dumb question and reveals my ignorance, but why do most functions not return a standard error? E.g. lm(Sepal.Length ~ Petal.Length, data = iris) %>% standardize_parameters
    could return a standard error in addition to CIs? It's OK if this is a choice by the package developers; I'm just thinking that many novice users would look for SEs, too.

Good question - I've tried to keep the output of all function to the Effect size + CI format, as not all effect sizes have accompanying SEs (at least that is not the method we've used for the CI estimation). However, specifically in standardize_parameters(), the SEs are actually retuned as an attribute:

library(magrittr)
library(effectsize)

(Z <- lm(Sepal.Length ~ Petal.Length, data = iris) %>% 
  standardize_parameters())
#> Parameter    | Coefficient (std.) |        95% CI
#> -------------------------------------------------
#> (Intercept)  |          -5.03e-16 | [-0.08, 0.08]
#> Petal.Length |               0.87 | [ 0.79, 0.95]
#> 
#> # Standardization method: refit

attr(Z, "standard_error")
#> [1] 0.04013870 0.04027317

I've made this explicit in the docs (under return). Thanks!
3a3e291#diff-1d1ffd135ac6eac822153973d2d9ec0c81e6812576fe2170aa3c6d4263069ff0R84-R86

  • Following the other review, I thought the package main branch is now called "main" instead of "master", but "master" still exists and appears ahead of "main". Is that intended? I see that issue Rename "master" branch to "main" branch #201 is still open, so you're most likely onto that?

Once the review is over (🤞) we will switch to main ( #210 ). I just didn't want to mess with the linking to JOSS.

  • In the JOSS paper's first paragraph, you might consider adding a reference to latent variables around the "(especially when units of measurement are not meaningful)". Standardized effect sizes are particularly useful when using latent factor scores, for example. Adding this might clarify the package's appeal to some additional readers.

Hmm, do you have any reference in mind? I've changed to "(especially when units of measurement are not meaningful, e.g. in the use of estimated latent variables)".
https://github.com/easystats/effectsize/pull/228/files?file-filters%5B%5D=.md#diff-e504eb580b095a7e65428b098183a581e475f0fb316db95287eacd7d4f344424R38

Issues I suggest considering before the article is accepted

  • It would be nice if the convert vignette had the first example data available to insert/reproduce, i.e. create it within the vignette.

I don't even know why it was echo=FALSE here! Fixed!
6075bae#diff-ef85b163a761a7c29eda485a4d3583278bc70e7d7e97d7d3a7566e1ecfb0c97fL44

  • The package help files ould describe the value of data frames that results from function calls better. Maybe an example would help the reader quickly use this data frame for further processing. This could also help with something like combining functions, such as a hypothetical call to interpret_d(standardize_parameters(mod)$Std_Median, rules = "cohen1988").

I've added more verbose information for the expected column names and such. Let me know if this is satisfactory. (3a3e291)

  • the paper could also mention the scale() function to standardize a matrix. That's what I was taught in my first R course many, many years ago :)

Great suggestion!!! Added to # Comparison to Other Packages
94744d8#diff-e504eb580b095a7e65428b098183a581e475f0fb316db95287eacd7d4f344424R46

Thoughts for future iterations of the project

  • Consider replacing examples using the iris data with another dataset, e.g. mtcars or penguins. See here for why.

This was gradually happening anyway, but now it's final - not a single iris in any of the docs / vignettes! (b250fe7)

  • A nice-to-have from my perspective would be if the output from effectsize could easily be passed on to other packages that allow automated output in the form of regression tables (texreg, stargazer, broom, etc.).

This is something that is in the works with papaja (see #156), and the report package (to hopefully be released soon)!

@jkarreth
Copy link
Author

@mattansb , Thanks for the quick turnaround! I'll check off issues in my review here in this issue to let you know what's addressed. Specific responses below:

In the JOSS paper's first paragraph, you might consider adding a reference to latent variables around the "(especially when units of measurement are not meaningful)". Standardized effect sizes are particularly useful when using latent factor scores, for example. Adding this might clarify the package's appeal to some additional readers.

Hmm, do you have any reference in mind? I've changed to "(especially when units of measurement are not meaningful, e.g. in the use of estimated latent variables)".
https://github.com/easystats/effectsize/pull/228/files?file-filters%5B%5D=.md#diff-e504eb580b095a7e65428b098183a581e475f0fb316db95287eacd7d4f344424R38

Quick suggestion (others might have better ideas?): p. 165 in Bollen, K. A. (1989). Structural Equations with Latent Variables. New York: Wiley

This is probably a dumb question and reveals my ignorance, but why do most functions not return a standard error? E.g. lm(Sepal.Length ~ Petal.Length, data = iris) %>% standardize_parameters
could return a standard error in addition to CIs? It's OK if this is a choice by the package developers; I'm just thinking that many novice users would look for SEs, too.

Good question - I've tried to keep the output of all function to the Effect size + CI format, as not all effect sizes have accompanying SEs (at least that is not the method we've used for the CI estimation). However, specifically in standardize_parameters(), the SEs are actually retuned as an attribute:
...

OK! Maybe you could put a note to this point in the JOSS paper so it's easier for readers to see? Just a suggestion - not a condition for me closing this review.

The package help files could describe the value of data frames that results from function calls better. Maybe an example would help the reader quickly use this data frame for further processing. This could also help with something like combining functions, such as a hypothetical call to interpret_d(standardize_parameters(mod)$Std_Median, rules = "cohen1988").

I've added more verbose information for the expected column names and such. Let me know if this is satisfactory. (3a3e291)

Yup, that's helpful! Resolved.

@jkarreth
Copy link
Author

One open issue from the initial review:

-[ ] .bib entry for @cohen1988statistical still has {\'A}/L} in the title field - I think that can go?

mattansb added a commit that referenced this issue Dec 10, 2020
mattansb added a commit that referenced this issue Dec 10, 2020
@mattansb
Copy link
Member

  • Fixed the Cohen (1988) citation (in the paper and across the whole package).
  • Added bollen1989structural ref

OK! Maybe you could put a note to this point in the JOSS paper so it's easier for readers to see? Just a suggestion - not a condition for me closing this review.

I think that is covered by the first line in the "Examples of Features": effectsize provides various functions for extracting and estimating effect sizes and their confidence intervals (and is evident in the code examples)? Is there any particular place you think this should be emphasized?


When you close this issue, I'll merge the PR #228 , and have the new PDF generated on the JOSS issue.
Thanks for taking the time!

@jkarreth
Copy link
Author

I think that is covered by the first line in the "Examples of Features": effectsize provides various functions for extracting and estimating effect sizes and their confidence intervals (and is evident in the code examples)? Is there any particular place you think this should be emphasized?

Maybe simply change to something like: effectsize provides various functions for extracting and estimating effect sizes and measures of uncertainty (such as confidence intervals and standard errors) ?

But I leave it to you; this is not a critical issue.

I'm closing the review here because all items above are addressed. Thanks! The only remaining item in my review over at JOSS is the "Functionality" checkpoint; I'll try a few more things in the next few days and will leave a note when that is checked off as well.

@jkarreth
Copy link
Author

Sorry to reopen, but looking at the re-compiled paper I noticed that a few references need to be fixed in the .bib file.

  • Some books are entered as @article instead of @book
  • Capitalization for titles and journal name is off in a few entries (e.g. Bmj)
  • Check hedges1985statistical and bollen1989structural for a few issues
  • If possible, please provide DOIs and ISBNs consistently.

Thanks for looking into that - and apologies for overlooking it earlier. Will close the issue here when I see a corrected .bib file.

@jkarreth jkarreth reopened this Dec 11, 2020
mattansb added a commit that referenced this issue Dec 11, 2020
@mattansb
Copy link
Member

@jkarreth I'm 99.9% I got all of them (see latest commit e2ec3b9) - let me know if I somehow missed anything 😅

@jkarreth
Copy link
Author

yup, I think all looks good now! Closing the review again.

@mattansb
Copy link
Member

Awesome - thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs 📑 Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants