-
-
Notifications
You must be signed in to change notification settings - Fork 36
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]: PySensors: A Python Package for Sparse Sensor Placement #2828
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @jordanperr, @tuelwer 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:
|
|
@jordanperr @tuelwer , make sure to accept the invitation to the reviewers group and to have a look at the reviewer guidelines linked to at the top of this review page. The review process will happen in this issue page, so questions to the author or to me can be added as comments here. |
@pdebuyl @briandesilva The authors propose the Python library PySensors which allows efficient data-driven sensor placement. The toolbox provides methods for reconstruction and classification based on the data of the sensors. The main contribution of the paper is the implementation of the sparse sensor placement optimization algorithm for reconstruction (SSPOR) and classification (SSPOC). Besides this, the toolbox provides useful bindings to existing methods. The API is designed in the style of scikit-learn which makes the toolbox very easy to use! The paper is well-written and gives a good overview over related work. However, I would have liked a more formal definition of the problem setting that is solved by the PySensors toolbox. The references seem to be complete. The scikit-learn JMLR paper has indeed no DOI stated on the website of the journal. There is a missing DOI for Chama which whedon found. The toolbox itself is well-documented. I especially like the example notebooks which are nicely written and give a good overview over the features of the toolbox. I was able to reproduce the results of the examples locally on my notebook as well as on binder. However, per default, binder is missing matplotlib, seaborn and pandas. Can this be configured? Installation of the toolbox on MacOS, Debian and Ubuntu through Regarding the community guide: I cannot see a clear statement on how third parties could seek support. I congratulate the authors to their great toolbox! Overall, I would recommend to accept this submission into JOSS. Minor remarks:
|
Thanks, @tuelwer! This is all very useful feedback. We'll work to update the paper and package accordingly. |
Thank you @tuelwer for this efficient review! |
@tuelwer, could you clarify which figure you're referring to here?
I believe I've addressed the other issues you raised, apart from the binder issue and the "more formal definition of the problem setting that is solved by the PySensors toolbox." I experimented with different solutions for specifying binder dependencies, but didn't come up with anything that worked. I'll have to look into both items further. |
@briandesilva What I meant was this example. I was indeed somewhat imprecise, sorry for this! |
Okay I've fixed the binder issue and the missing image. @tuelwer, with regards to your comment
there are different objective functions that are approximately optimized by PySensors classes depending on the problem type. We had hesitated to include them in our JOSS submission based on our understanding that the paper is meant to be aimed at a general audience. What do you think about the idea of adding the objective functions to their respective Jupyter notebooks? |
@briandesilva, thanks! Except for the sea surface temperature example (FTP connection problem) all notebooks now run smoothly on Binder! Regarding the summary: I agree that the paper should be aimed at a general audience (as it is also required by the JOSS submission guidelines) and I agree that stating the loss functions in the paper is probably too much. However, maybe you could consider to elaborate on the data driven aspect of your toolbox, which you briefly describe in the second paragraph of your summary. For example, I found Figure 1 of [1] and the gray box on page 2 of [2] very helpful to understand the problem setting. In my opinion, adding more details of the problem setting would greatly improve the quality of the paper since it would be easier for a user to assess whether PySensors is suitable for their problem. References |
Review for PySensors: A Python Package for Sparse Sensor Placement Pysensors is a Python implementation of the SSPOC (pysensors.classification.SSPOC) and SSPOR (pysensors.reconstruction.SSPOR) methods to perform optimization of sensor placement. These methods are published in (Brunton, 2016) which is cited in both the documentation and in the paper. The repository under review contains documentation (hosted on readthedocs and in Readme markdown files) and worked out examples in the form of Jupyter notebooks. The code is thoughtfully organized, with two optimizers (CCQR and QR) that extend scikit-learn's BaseEstimator, and two utility functions that wrap MultiTaskLasso and OrthogonalMatchingPursuit from scipy. I was able to install the software, run the tests, and execute some of the example notebooks. The paper is well written and fits within the scope of the journal. I believe the code represents a significant contribution as a user friendly implementation of the SSPOR and SSPOC algorithms. I would have liked to see citations of academic work that use the PySensor package. Such citations would provide evidence of the impact of this software in an academic field. Overall, I recommend acceptance for publication after these minor comments are reviewed and implemented at the author's discretion: Paper:
Documentation:
Examples:
Code:
|
Thank you for the review @jordanperr ! |
Hi @briandesilva make sure to update us here on the progress so far. |
First I'd like to thank @jordanperr for your comprehensive review of our package. I have just pushed changes that I believe address your comments under Documentation, Examples, and Code. A couple follow-up items:
Do you have any suggestions for how we should address this? One option could be to narrow the versions of python the package is compatible with to 3.6-3.8 until Scikit-learn releases python 3.9 binaries.
This is the style used in Sckit-learn. For example, see the linear_model module. |
Hi @briandesilva thanks for the update. Did you address all concerns at this point so that the reviewers can take a look at the project again? |
@whedon generate pdf |
@briandesilva, @kmanohar thank you for adding the description of the problem setting. Some minor remarks: the index i is not defined. Perhaps you want to add |
We've yet to address Jordan's comments regarding the paper. I'll post here once the associated changes have been made. |
Thanks @briandesilva . Can you edit the metadata of the zenodo archive to match the paper please? Title should be "PySensors: A Python Package for Sparse Sensor Placement" and authors should be as here, with ORCID properly set. |
@whedon set 10.5281/zenodo.4542530 as archive |
OK. 10.5281/zenodo.4542530 is the archive. |
@whedon set v0.3.3 as version |
OK. v0.3.3 is the version. |
@pdebuyl, I've updated the archive as you asked. |
Thanks @briandesilva |
@whedon accept |
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#2089 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#2089, then you can now move forward with accepting the submission by compiling again with the flag
|
|
For the eic: only orcid ids for authors seem missing from metadata. |
I just added Orcid IDs for the authors to the paper. Sorry, I hadn't thought to add them when I added them to the DOI archive. |
Thanks @briandesilva . I don't think that we require them, I mentioned it as I believe that it brings value to the authors, to the readers, and to the journal :-) The submission is in the hands of the Editors-in-chief in rotation. |
@whedon accept |
|
👋 @openjournals/joss-eics, this paper is ready to be accepted and published. Check final proof 👉 openjournals/joss-papers#2097 If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#2097, then you can now move forward with accepting the submission by compiling again with the flag
|
|
@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... |
@jordanperr, @tuelwer - many thanks for your reviews here and to @pdebuyl for editing this submission. JOSS relies upon the volunteer efforts of folks likes yourselves and we simply wouldn't be able to do this without you! ✨ @briandesilva - your paper is now accepted and published in JOSS ⚡🚀💥 |
🎉🎉🎉 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:
|
@arfon, I'm really sorry, but it was just pointed out to me that two of the author names were written in the wrong order in the paper. Is there any way to re-generate the pdf with the corrected order? I have corrected and pushed |
Sure thing. Updated in openjournals/joss-papers@8f2fc95 |
Submitting author: @briandesilva (Brian de Silva)
Repository: https://github.com/dynamicslab/pysensors/
Version: v0.3.3
Editor: @pdebuyl
Reviewer: @jordanperr, @tuelwer
Archive: 10.5281/zenodo.4542530
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
@jordanperr & @tuelwer, 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 @pdebuyl 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 @jordanperr
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @tuelwer
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
The text was updated successfully, but these errors were encountered: