Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upCode Comments for R Journal Review #1
Comments
This comment has been minimized.
This comment has been minimized.
When you have a constrained set of character string values that are allowed for a function argument, like in That prevents you from having to write code like: https://github.com/shuowencs/SortedEffects/blob/master/R/PEestimate.R#L19 |
This comment has been minimized.
This comment has been minimized.
Rather than giving errors that ask for a correct argument value, the errors might be improved by saying what values are allowed (e.g., https://github.com/shuowencs/SortedEffects/blob/master/R/u.Subpop.R#L48 and subsequent lines). |
This comment has been minimized.
This comment has been minimized.
For bootstrap draws, why set a default value that you don't personally recommend? https://github.com/shuowencs/SortedEffects/blob/master/R/u.Subpop.R#L26 |
This comment has been minimized.
This comment has been minimized.
Explicitly return the And the |
This comment has been minimized.
This comment has been minimized.
Missing variable definitions for |
This comment has been minimized.
This comment has been minimized.
If plotting functions are meant to be convenient quick-use tools for the interactive user, I would be inclined to set default values to arguments like That's done in some places (e.g., https://github.com/shuowencs/SortedEffects/blob/master/R/SPE.R#L223) but it would be good to be consistent across the whole package. |
This comment has been minimized.
This comment has been minimized.
It's generally bad practice to set options() inside functions (https://github.com/shuowencs/SortedEffects/blob/master/R/PEestimate.R#L17). If you do it, you should reset to the previous value. |
leeper commentedOct 2, 2019
I am a reviewer of your manuscript for R Journal and am submitting a few comments on code here.