-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
[REVIEW]: wbacon: Weighted BACON algorithms for multivariate outlier nomination (detection) and robust linear regression #3238
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @msalibian, @aalfons it looks like you're currently assigned to review this paper 🎉. Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post. ⭐ Important ⭐ If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿 To fix this do the following two things:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
|
@msalibian and @aalfons - we're now starting the review. You can see above your review checklist. Please check the boxes as you proceed through the review. For any boxes that you can't check right now, please leave comments in this thread. You may also choose to open issues in the package repository. If you do open issues, please be sure to reference them in this thread. Please let me know if you have any questions as you work through the review. Thanks again! |
@msalibian and @aalfons - how is the review going? Is there anything that I can assist with? Please feel free to check off the boxes above as you proceed through the review. Thanks! |
This is a nice contribution (both package and manuscript). I have some minor suggestions about the text, and noted that some items from the checklist above are missing.
|
@msalibian - Thank you for the very helpful remarks. Before I’m going to address the points raised, I have some questions for the editor. @fboehm - I have the following questions:
|
@tobiasschoch - Thanks for your questions. Below I try to answer them.
|
@aalfons - how is the review going? Please let me know if you encounter any difficulties. |
@fboehm @tobiasschoch About the above: 2 - I don't have a strong opinion on this. A reference to the vignette would probably be enough. Although in that case I think it would be helpful to include in the paper a few sentences describing / quantifying the performance gain that can be expected with this implementation. Something like "When the sample size is larger than XXX, using this implementation often results in a speed gain of YYY% compared with such and such..." 4 - Agreed. |
@fboehm The only difficulty is finding time. I'll get to it by the end of the week. My apologies for the delay, I was not aware that it needs to be done so quickly. |
|
@aalfons - I apologize if I implied that I expected the review to be done by now. That wasn't my intention. I merely wanted to ask if you'd gotten started or encountered difficulties. Sometimes reviewers are unsure how to start the review, especially since we use a nontraditional format at JOSS. If any questions arise, please let me know. Thanks again!! |
@tobiasschoch - that all sounds good. Please feel free to address the issues that @msalibian raised as the next step. Thanks!! |
👋 @msalibian, please update us on how your review is going (this is an automated reminder). |
👋 @aalfons, please update us on how your review is going (this is an automated reminder). |
The package seems to provide useful functionality and is well documented, and the manuscript describes the functionality and the need for it nicely. I still have some comments, some of which I realized afterwards are also brought up by @msalibian.
In file included from fitwls.c:20: The latest version of R for Mac now rely only on the compilers shipped with Apple's XCode developer platform, and unfortunately it seems that Apple has disabled OpenMP support there by default, see https://mac.r-project.org/openmp/ . I was not aware of that. I have of course fixed OpenMP support now as per instructions on the above website, and the package installed as per the documentation. But there may be other users who run into the same issue and give up on your package. I believe the recommended way to include OpenMP in the C++ header files is the following: #ifdef _OPENMP This is a simple fix, so please go ahead and implement it. While users without OpenMP installed then do not have access to parallel computing when using your package, at least they can install and use it.
General comments:
|
I modified the paper according to @msalibian suggestions (I'm going to address the points raised by @aalfons later) Issue 1
Issue 2
Issue 3
Issue 4
Issue 5
Issue 6
Issue 7
Issue 8
|
@whedon generate pdf |
@aalfons - Thank you for the very helpful remarks. Your comments reached me while I was responding to the points raised by msalibian. I will now address the points you have raised. Issue 1Reviewer: Installation instructions: I received the following error message when trying to install the package [...]. Answer: Yes, indeed I forgot to add the (conditional) include guards. This is now fixed. I added the include guards in the C header files. Issue 2Reviewer: Performance: line 36: What size of the data set (number of observations, number of variables) is approximately necessary for implementation inefficiencies to matter? Answer: In the mean time I have updated the paper. In the "Benchmarking" section, I compare my implementation with the reference implementation Issue 3Reviewer: Automated tests: There are some scripts in the tests/ directory that seem to reproduce the results of a paper and contain some speed tests of the computational performance, but I did not find any unit tests that the functions produce the correct output and so on. On the other hand, the documentation contains some examples, and it can be automatically checked whether these produce warnings or errors via R CMD check. So in that sense, there would be some minimal automated testing of the functionality. @fboehm Could you please provide some guidance on this point? Answer: I am a believer in test-driven development. I distinguish between two (idealized) types of tests:
As a pragmatic programmer I take the following view: Some functions are suitable for unit testing, others less so.
We could, in principle, follow the unit-test paradigm and write separate unit tests for the covariance matrix, regression, and so on. However, this would ultimately boil down to testing the subroutines in BLAS and LAPACK. Neither is my package the place to test these subroutines, nor would such a testing strategy increase confidence in my functions. Of course, that does not rule out the possibility that I mixed up the order of the arguments when calling the subroutines. But the compiler (and valgrind) would have told me if I had actually done it. So I decided to take a "functional test" approach and test the ensemble of building blocks. In total, the BACON algorithms are tested on 159 different (real) test sets to match the results of the (reference implementation) in the Issue 4Reviewer: State of the field: The authors mention only other packages that implement the same algorithms. Maybe it would be good to also mention some other popular packages that provide other algorithms for anomaly detection? (Although this could quickly go out of hand, as there are many algorithms for this purpose.) Answer: Yes, this could indeed go out of hand quickly... I focused on the BACON algorithms and did not intend to write a review article on multivariate outlier detection. @fboehm: Shall I discuss other methods? Issue 5Reviewer: line 12: the statement that the BACON algorithms are "superior" would imply that they are always better than other anomaly detection algorithms. Please rephrase this. Answer: Yes, this has been fixed; see response to points raised by msalibian. Issue 6Reviewer: lines 15-17: Even though the function to load a package is called library(), which causes some confusion about the terminology, the preferred term is "package" in R. Please change this throughout the paper. Answer: Yes, this has been fixed; see response to points raised by msalibian. Issue 7Reviewer: line 66/67: How should one proceed if the diagnostic tools indicate that the "good" observations violate the structure that is required by the algorithm? Answer: Well... all I can do (see also vignette) is to quote the developpers of the BACON algorithms: “Although the algorithms will often do something reasonable even when these assumptions are violated, it is hard to say what the results mean.” Billor et al. (2000, p. 290). It is better to be "safe than sorry" and apply another method. I think I have made it sufficiently clear when the method can and cannot be used; see vignette. |
I'm sorry human, I don't understand that. You can see what commands I support by typing:
|
@whedon set v0.5 as version |
OK. v0.5 is the version. |
@whedon check references |
|
@whedon set 10.5281/zenodo.4895167 as archive |
OK. 10.5281/zenodo.4895167 is the archive. |
@whedon accept |
To recommend a paper to be accepted use |
@whedon recommend-accept |
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#2356 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#2356, then you can now move forward with accepting the submission by compiling again with the flag
|
|
@tobiasschoch - I just recognized that the title of the archive doesn't match the title of the manuscript. Can you please fix this? |
@fboehm - Sorry... I have fixed it. |
|
|
@whedon generate pdf |
@whedon accept deposit=true |
|
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
Congratulations @tobiasschoch on getting this work published in JOSS! Thanks @fboehm for editing this work! Thanks also to @msalibian, @aalfons for the great job reviewing this work! |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
Submitting author: @tobiasschoch (Tobias Schoch)
Repository: https://github.com/tobiasschoch/wbacon
Version: v0.5
Editor: @fboehm
Reviewer: @msalibian, @aalfons
Archive: 10.5281/zenodo.4895167
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@msalibian & @aalfons, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @fboehm know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Review checklist for @msalibian
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @aalfons
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: