Open andrealee011 opened 4 years ago
Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
Estimated hours spent reviewing:
I like the code style; it's very simple and easy-to-read!
However, there are a few problems with your project, structure wise.
there are a few edge-cases that are covered in rb4model.R
, but no corresponding tests.
rb4model.R
testthat
shouldn't be in the list of dependencies in README.md
; it should be a dev dependency instead.
rb4model.Rproj
shouldn't be in the repo
there are no instructions on how to use the package, other than the documentation of the functions; I was able to run your code after copy+pasting your vignette.Rmd
, but that wasn't obvious (see below)
not sure where the Vignette is? I see the .Rmd file, but not sure where it's knitted to. There's also a docs folder, but no docs are linked in README.md
, and your *.yml
files don't mention any sort of build either.
build.yml
doesn't actually do anything; it looks like you might want to replace it with R-CMD-Check.yml
from R-CMD-Check.yml
's latest build:
'datasets' 'e1071' 'mlbench' 'randomForest'
All declared Imports should be used.
These libraries need to be removed from DESCRIPTIONS
.
Hi rb4model team, I was assigned as a reviewer of your R package and I am pleased to share the following review and recommendations:
Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [X] A short summary describing the high-level functionality of the software
- [X] Authors: A list of authors with their affiliations
- [X] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
Estimated hours spent reviewing:
I found your package very complete in the sense that have complementary functions that help you to perform an appropriate data pre-processing. I believe that the idea is very good and you could improve your package mainly sharing more information for the user.
feature_splitter
function splits the features into two categories features; however, it doesn’t say which of those are numerical and which are categorical. For example, I made a data frame with 3 numerical an 2 categorical variables, and the function separate them well but it doesn’t tell me which belomg to each cathegory:
x <- data.frame('X1'=c(1,2,3,4), 'X2'=c('a','b','c','d'), 'X3'=c(11,12,13,14),'X4'=c(90,99,120,421), 'X5'=c('ax','bx','cx','dx'))
feature_splitter(x)
[[1]] [1] "X2" "X5"
[[2]] [1] "X1" "X3" "X4"
- Forward Selection function: When I run the `ForwardSelection`, if I change the minimum number of features it doesn’t return an appropriate output of what I am asking. In the next example I ask at least for three features and I get only one as a result:
```{r}
y <- iris$Species
x <- iris[c(1,2,3,4)]
ffs <- ForwardSelection(feature=x, label=y, my_mod="rf", min_f = 2)
ffs
[1] 2
print(head(x[ffs], 10))
[output]:
Sepal.Width
1 3.5
2 3.0
3 3.2
4 3.1
5 3.6
6 3.9
7 3.4
8 3.4
9 2.9
10 3.1
I hope you find this review useful!
Thank you for the feedback! It was extremely hepful! Because of time constraints, we were unable to address all your feedback. Here is a list of the changes we did make:
Please find our new relase here: https://github.com/UBC-MDS/rb4model/tree/v1.1.1
Thank you @v5y8 and @vcuspinera for your valuable feedback. We have made the changes outlined above by @andrealee011. I just wanted to add one more comment on the feature_splitter
for the use of OHE. The intended use of the function was to help users visualize what kind of data they are dealing with, i.e. which ones are numerical and which ones are categorical. Kind of quick EDA for them. Not necessarily to do full data preprocessing. But we have changed its output into a data frame which is more presentable to users.
Submitting Author: Andrea Lee (@andrealee011 ), Jaekeun Lee (@agdal1125), Sakariya Aynashe (@eyrakas), Xinwen Wang (@xiw315 ) Repository: rb4model Version submitted: v1.1.0 Editor: Varada Kolhatkar (@kvarada) Reviewer 1: Yi Liu (@v5y8)
Reviewer 2: Victor Cuspinera-Contreras (@vcuspinera) Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
Who is the target audience and what are scientific applications of this package?
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
JOSS Options
- [ ] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: - (*Do not submit your package separately to JOSS*)MEE Options
- [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)Code of conduct