-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
The Database of Odorant Responses (DoOR.functions) #34
Comments
Thanks for your submission, seeking reviewers |
Reviewers: @RemkoDuursma |
@RemkoDuursma |
@RemkoDuursma |
Hi @RemkoDuursma, just checking in on this review, which was due 2016-05-09. Please get it in soon, thanks! |
@RemkoDuursma |
Apologies for my very slow review.
|
@Dahaniel can you respond to the review above please |
@RemkoDuursma Thank you very much for reviewing the package and apologies for my slow response.
We also use if (DoOR_default == "DoOR.mappings") {
if (!exists("DoOR.mappings")) {
data(DoOR.mappings)
}
return(DoOR.mappings)
} The idea is that the user can put modified data into the global environment that will be used when available. Not sure whether this could be implemented with options.
|
Hi everyone, apologies that it took me so long to implement the requested changes. But finally today I was able to release v2.0.1 for both packages, implementing all of the above discussed changes.
|
thx @Dahaniel @RemkoDuursma if possible, could you take a look at the changes for this and #35 and let us know if you're happy with the changes? also let us know if you're too busy/etc, and we'll have a look instead |
Sorry I won't have a chance any time soon to look at the changes. If you could have a look, that would be great. |
Thanks for the update, @Dahaniel. As @RemkoDuursma is no longer available, we'll have an alternate reviewer or editor look at your changes. |
Some of R check output
many problems above:
|
Thanks for the comments @sckott
That is right, as explained before I do not have a clue about unit testing and
True, in an earlier mail you suggested to put the data package into DEPENDS, I
ok
As discussed in #35 (comment) this is how the package was initially written: "Load data into the As described, my workaround for now was to create a user interaction upon
These are from the
Many of the arise from using ## quiets concerns of R CMD check re: the .'s that appear in pipelines
if(getRversion() >= "2.15.1") utils::globalVariables(c(".")) here STAT545-UBC/Discussion#451 summaryPoints 1,3 & 7 I think I can fix. I would need some advice to fix |
@Dahaniel okay, let me know when you're done |
@Dahaniel anything to report on this? |
@sckott I implemented unit testing in the data package, for the functions package I scheduled implementation for 1st half of August. |
Okay, thanks much |
just checking in @Dahaniel - i'm waiting on this correct:
|
@sckott quick update: I am working on this but it'll take a few more days. |
Ok, should be done. I fixed:
there remains:
|
For the data issue: isn't it possible to do this approach:
this should get around the problem of the package loading data into the global namespace i think we do need to solve this issue as CRAN will likely not accept it as is I think the circularity thing is actually okay - as long as one has the other in Suggests |
There's a few more things to fix. These are all quite important to fix. GP DoOR.functions ─────
It is good practice to
✖ omit "Date" in DESCRIPTION. It is not required and it
gets invalid quite often. A build date will be added to the package
when you perform `R CMD build` on it.
✖ avoid long code lines, it is bad for readability. Also,
many people prefer editor windows that are about 80 characters
wide. Try make your lines shorter than 80 characters
R/back_project.R:53:1
R/back_project.R:56:1
R/back_project.R:65:1
R/back_project.R:68:1
R/back_project.R:69:1
... and 434 more lines
✖ avoid sapply(), it is not type safe. It might return a
vector, or a list, depending on the input data. Consider using
vapply() instead.
R/calculate_model.R:48:19
R/get_dataset.R:25:15
R/get_dataset.R:26:22
R/get_responses.R:36:36
R/import_new_data.R:94:29
... and 12 more lines
✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...),
1:NROW(...) and 1:NCOL(...) expressions. They are error prone and
result 1:0 if the expression on the right hand side is zero. Use
seq_len() or seq_along() instead.
R/count_studies.R:31:13
R/door_melt.R:29:13
R/dplot_tuningCurve.R:82:35
R/dplot_tuningCurve.R:110:36
R/get_responses.R:33:13
... and 19 more lines
✖ avoid 'T' and 'F', as they are just variables which are
set to the logicals 'TRUE' and 'FALSE' by default, but are not
reserved words and hence can be overwritten by the user. Hence,
one should always use 'TRUE' and 'FALSE' for the logicals.
R/back_project.R:NA:NA
R/get_dataset.R:NA:NA
R/map_receptor.R:NA:NA
R/rebuild_metadata.R:NA:NA |
Thanks @sckott , how can I reproduce the good practice output you posted here? |
install |
any thoughts @Dahaniel ? |
Hi @sckott , sorry, busy as usual. |
@Dahaniel Okay, did you get to fixing these goodpractice problems yet? |
@sckott Should be fixed for both packages, shortened code-lines, replaced sapply with vapply etc... |
@Dahaniel thanks, having a look |
Sorry for the very long wait @Dahaniel ! Approved! Thanks again for your submission
|
That's awesome news! I'll see to your points in the coming days and I am happy to write a blog-post as well. |
@sckott where should I look for the invitation to the ROpenSci group? Could not find anything on github or in my mails. |
sorry about that, you should see a notification now or soon |
Hello @Dahaniel. Great to hear you're interested in writing a blog post about this. Here are some technical and editorial notes for contributing a post: https://github.com/ropensci/roweb2#contributing-a-blog-post We ask that you submit a draft via pull request at least one week prior to the planned publication date. I have two dates open right now. Take your pick:
|
Thanks @stefaniebutland, I pick March 20th then! |
Let me know if I can help along the way |
@stefaniebutland is there a guideline or a specific structure that I should follow when writing the blog post? |
@Dahaniel No specific structure. The best approach might be to read a few other blog posts about onboarded packages and see which ones you like / which ones have a narrative that grabs you. This blog tag gets you to some posts: https://ropensci.org/tags/review/ Some basic suggestions here: https://github.com/ropensci/roweb2#blog-post-editorial-suggestions |
If it would help, I could give you feedback on an outline where you identify audience, take home message and key points you want to get across. No obligation to do this |
Package containing the functions for the DoOR project. DoOR provides a framework to merge heterogeneous response data into a single consensus database, thus creating a comprehensive view on olfactory coding.
https://github.com/Dahaniel/DoOR.functions
It provides its own data via the
DoOR.data
package (see corresponding onboarding issue).Scientists working on olfactory coding, modelers and physiologists.
No, to our knowledge DoOR is the only project providing this kind of data and using this approach to integrate heterogeneous data.
devtools
install instructionsdevtools::check()
produce any errors or warnings? If so paste them below. - noThe text was updated successfully, but these errors were encountered: