[REVIEW]: PySensors: A Python Package for Sparse Sensor Placement

whedon commented 3 years ago

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

:warning: JOSS reduced service mode :warning:

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:

HTML: <a href="https://joss.theoj.org/papers/f49d5ede5060291a29f76274adde9a65"><img src="https://joss.theoj.org/papers/f49d5ede5060291a29f76274adde9a65/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/f49d5ede5060291a29f76274adde9a65/status.svg)](https://joss.theoj.org/papers/f49d5ede5060291a29f76274adde9a65)

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:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@briandesilva) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @tuelwer

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@briandesilva) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 3 years ago

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 :tada:.

:warning: JOSS reduced service mode :warning:

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.

:star: Important :star:

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:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/JSEN.2018.2887044 is OK
- 10.1109/mcs.2018.2810460 is OK
- 10.1137/15M1036713 is OK
- 10.1017/jfm.2011.195 is OK
- 10.2514/6.2004-2415 is OK
- 10.1073/pnas.1517384113 is OK
- 10.1109/access.2018.2886528 is OK
- 10.1109/access.2020.3023625 is OK
- 10.1126/science.1165893 is OK
- 10.1103/physrevmaterials.2.083802 is OK
- 10.1111/j.2517-6161.1996.tb02080.x is OK
- 10.5281/zenodo.1173754 is OK
- 10.1364/oe.24.030433 is OK
- 10.1063/1.5066099 is OK
- 10.1063/1.4977057 is OK
- 10.1016/j.ymssp.2018.08.033 is OK
- 10.1126/sciadv.1602614 is OK
- 10.1098/rspa.2016.0446 is OK
- 10.1137/16m1086637 is OK
- 10.1137/18m116798x is OK
- 10.1017/jfm.2017.823 is OK
- 10.1063/1.5018409 is OK
- 10.1016/j.ifacol.2016.10.249 is OK
- 10.1103/physreve.96.023302 is OK
- 10.1016/j.jcp.2018.10.045 is OK
- 10.1098/rspa.2018.0335 is OK
- 10.1016/j.jcp.2019.07.049 is OK
- 10.1007/s00162-020-00536-w is OK
- 10.1103/physreve.101.010203 is OK
- 10.1115/1.4043148 is OK
- 10.1016/j.ocemod.2009.01.001 is OK
- 10.1109/cdc.2014.7040017 is OK
- 10.1017/jfm.2017.137 is OK
- 10.1073/pnas.1808909115 is OK
- 10.1016/j.jmsy.2018.01.011 is OK
- 10.1017/jfm.2018.147 is OK
- 10.1017/9781108380690 is OK
- 10.1007/s00162-020-00520-4 is OK
- 10.1002/cpa.20124 is OK
- 10.1016/0167-7152(84)90020-8 is OK
- 10.1016/j.crma.2004.08.006 is OK
- 10.1162/0899766053723032 is OK
- 10.1109/TIT.2006.871582 is OK
- 10.1109/tit.2006.885507 is OK
- 10.1109/tit.2005.862083 is OK
- 10.1109/MSP.2007.4286571 is OK
- 10.1109/tit.2009.2034811 is OK
- 10.1016/j.acha.2010.10.002 is OK
- 10.1145/1879141.1879192 is OK
- 10.1137/090766498 is OK
- 10.1111/j.1467-9868.2011.00783.x is OK
- 10.1137/110822724 is OK
- 10.1137/15M1019271 is OK
- 10.1109/tsipn.2016.2614903 is OK
- 10.1109/sam.2016.7569707 is OK
- 10.1137/16m1081270 is OK

MISSING DOIs

- 10.2172/1405271 may be a valid DOI for title: Sensor placement optimization using Chama

INVALID DOIs

- 10.5555/1953048.2078195 is INVALID

pdebuyl commented 3 years ago

@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.

tuelwer commented 3 years ago

@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 pip install . and pip install python-sensors worked without problems. All tests passed with some warnings. There were some minor problems with the reconstruction example in the README.rst which are described in this issue.

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:

The last reference in the README.rst is printed in bold. Is this done on purpose?
In example notebook pysensors_overview.ipynb two different UserWarnings are thrown.
Some example notebooks require additional packages like seaborn or netCDF4. It would be nice to have a requirements-examples.txt to install all those packages in a single run.
Figure in the reconstruction example of the docs is missing.
"the number of possible placements grows combinatorially" (first paragraph in paper): "combinatorially" -> "exponentially"

briandesilva commented 3 years ago

Thanks, @tuelwer! This is all very useful feedback. We'll work to update the paper and package accordingly.

pdebuyl commented 3 years ago

Thank you @tuelwer for this efficient review!

briandesilva commented 3 years ago

@tuelwer, could you clarify which figure you're referring to here?

Figure in the reconstruction example of the docs is missing.

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.

tuelwer commented 3 years ago

@briandesilva What I meant was this example. I was indeed somewhat imprecise, sorry for this!

missing_image

briandesilva commented 3 years ago

Okay I've fixed the binder issue and the missing image. @tuelwer, with regards to your comment

I would have liked a more formal definition of the problem setting that is solved by the PySensors toolbox

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?

tuelwer commented 3 years ago

@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 [1] Brunton, Bingni W., Steven L. Brunton, Joshua L. Proctor, and J Nathan Kutz. "Sparse sensor placement optimization for classification." SIAM Journal on Applied Mathematics 76.5 (2016): 2099-2122. [2] Manohar, Krithika, Bingni W. Brunton, J. Nathan Kutz, and Steven L. Brunton. "Data-driven sparse sensor placement for reconstruction: Demonstrating the benefits of exploiting known patterns." IEEE Control Systems Magazine 38, no. 3 (2018): 63-86.

briandesilva commented 3 years ago

@kmanohar, do you want to take a stab at adding a brief description of the problem setting to the paper?

jordanperr commented 3 years ago

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:

Citation would be useful for the claim “In general, choosing the globally optimal placement… is an intractable computation”.
The notion of "sensors" and "optimization of sensor placement" could be considered as domain specific jargon for a truly general audience. The authors may consider defining these terms so the general reader has a more concrete picture of the problem being solved here.
A note on the difference between “detection” and “reconstruction and classification” tasks would be helpful.
In "statement of need," it may be construed from the text that Chama is the only other software to solve the sparse sensor placement problem. Quickly searching "sensor placement" on Github yields 77 repositories, including SSPOR_pub repo from K. Manohar (an author of this paper), which appears to implement the SSPOR algorithm in MATLAB. Some other repos include "sustainability-lab/polire", and "Chandrayee/sensor-placement". How do these codes fit into the story?
- The authors could mention how optimal sensor placement is currently solved by each researchers with custom scripts on top of more general statistical learning and optimization packages, such as scikit-learn or scipy. A brief overview of the more general techniques such as SVD, POD, or DMD would be beneficial and help explain why choosing PySensors is superior to a custom script.
Citations of academic work that use PySensors could help prove the impact of the software. I do believe that researchers in my peer group would use this software (and I will certainly send it around to them!)

Documentation:

At time of review, binaries for the scikit-learn dependency for Python 3.9.0, Mac OSX, are not available. The installation thus fails trying to compile scikit-learn from source. I specified python 3.8.5 and the installation worked as expected.
The readme assumes familiarity with Python and pip. This assumption is reasonable, but may not be valid for all academic users.
- Consider including a high level description of the dependencies in the Readme, such as “Python 3.6-3.8 with Mac or Linux and pip”. Not immediately clear if this software supports Windows.
- May not be clear for all users what the next step is after "pip install ." A simple sentence with a link to the examples section would help.
There is something weird going on with the TOC on the ReadTheDocs page. I see "pysensors.version module" and "Module contents" as subheadings under "API" - which doesn't seem right. Otherwise, the RTD page looks very good.

Examples:

The example notebooks have a lot of warnings. To pick on one: classification.ipynb (as checked in to github) has the following warning after cell 19:

/home/brian/Documents/Dropbox (uwamath)/Brian/Research/PySensors/pySensors/venv/lib/python3.6/site-packages/sklearn/linear_model/_coordinate_descent.py:1914: UserWarning: Coordinate descent with l1_reg=0 may lead to unexpected results and is discouraged.
check_random_state(self.random_state), random)

Code:

Tests pass with 36 warnings. They all look like the aforementioned "UserWarning" caused by improper parameters / small data, which is to be expected with tests. These warnings could be caught by the tests and ignored.
Performance is not mentioned in the documentation. Would be useful to know the time and space complexity of these algorithms.
Excellent code style and software architecture. I believe the implementation is exemplary, although I am not familiar with the pattern of naming each module with an underscore prefix and then renaming them in the __init__ scripts.

pdebuyl commented 3 years ago

Thank you for the review @jordanperr !

pdebuyl commented 3 years ago

Hi @briandesilva make sure to update us here on the progress so far.

briandesilva commented 3 years ago

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:

At time of review, binaries for the scikit-learn dependency for Python 3.9.0, Mac OSX, are not available. The installation thus fails trying to compile scikit-learn from source. I specified python 3.8.5 and the installation worked as expected.

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.

I believe the implementation is exemplary, although I am not familiar with the pattern of naming each module with an underscore prefix and then renaming them in the init scripts.

This is the style used in Sckit-learn. For example, see the linear_model module.

pdebuyl commented 3 years ago

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?

tuelwer commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

tuelwer commented 3 years ago

@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 for i=1...n or something similar. Also it would be nice to state the shape of y, C and x and to make clear that y usually has less entries than x.

briandesilva commented 3 years ago

@pdebuyl

Did you address all concerns at this point so that the reviewers can take a look at the project again?

We've yet to address Jordan's comments regarding the paper. I'll post here once the associated changes have been made.

briandesilva commented 3 years ago

@jordanperr @tuelwer, thanks again for all your feedback. We have pushed a new version of the paper in which we have attempted to address your comments.

Some other repos include "sustainability-lab/polire", and "Chandrayee/sensor-placement". How do these codes fit into the story?

We were unable to find any documentation for the package "Chandrayee/sensor-placement" and hence did not include it in the paper, but based on the code, it appears to optimize the same Gaussian process criteria as "sustainability-lab/polire".

Citations of academic work that use PySensors could help prove the impact of the software.

As far as we know, PySensors has not been used in any published academic work as of yet.

briandesilva commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

tuelwer commented 3 years ago

@briandesilva thanks for clarifying the description!

@pdebuyl all my comments have been addressed appropriately. I see no further issues and I recommend to accept this submission.

pdebuyl commented 3 years ago

Thank you @tuelwer for the review!

@briandesilva some of @jordanperr comments remain not entirely answered, as I understand it. A note on the performance is missing, for instance.

Also, could you state that the code is untested under windows, if that is well the case?

Some papers lack a DOI, such as 10.1137/17M1162366 (even if whedon did not list it, you should check that everything that has a DOI is listed with the DOI in the paper).

briandesilva commented 3 years ago

@pdebuyl, I have fixed the DOIs and added a line to the README about lack of testing for windows machines.

I will have to go back and look over @jordanperr's comments to check for anything I've missed. Though I'll note that I previously added performance discussions to various sections of the documentation.

pdebuyl commented 3 years ago

Hi @briandesilva the note on windows is fine, thank you. Regarding performance, I only looked at the paper, it is fine in the doc sorry.

briandesilva commented 3 years ago

Hi @pdebuyl, I've looked back over @jordanperr's feedback and saw that the following comment may not have been completely addressed:

The notion of "sensors" and "optimization of sensor placement" could be considered as domain specific jargon for a truly general audience. The authors may consider defining these terms so the general reader has a more concrete picture of the problem being solved here.

I've added a brief description of a sensor to the first sentence of the paper. I think our other recent updates help clarify what we mean by "optimization of sensor placement", but please let me know if you feel otherwise.

pdebuyl commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pdebuyl commented 3 years ago

@briandesilva In basis_comparison notebook, I had to remove quiet=True from update_n_basis_modes function. Probably related to changes in the dependencies, can you check?

@jordanperr can you have another look at the code+paper and let us know whether you consider your comments addressed for publication?

briandesilva commented 3 years ago

@pdebuyl, I just created a new release of the package—see if updating fixes the issue. I wasn't able to reproduce the problem on this version (0.3.1).

pdebuyl commented 3 years ago

Fixed. I used the version from pip earlier, there might have been a mismatch between the pip installed and the git-downloaded examples.

pdebuyl commented 3 years ago

@jordanperr the review is almost complete, can you check the checklist (3 items remaining)?

When everything is ok, I'll also need an explicit statement of acceptance.

pdebuyl commented 3 years ago

@jordanperr gentle reminder.

pdebuyl commented 3 years ago

@jordanperr gentle reminder

pdebuyl commented 3 years ago

@jordanperr gentle reminder.

jordanperr commented 3 years ago

Well, looks like my Github notifications are not configured properly.. My sincere apologies for holding this up. Thanks @pdebuyl for sending me a nudge over email.

@briandesilva, it appears the dependency issue with Python 3.9 has resolved itself, so I have no further suggestions with respect to that issue. It looks like some statements on algorithmic complexity have been added to the documentation, which is great, but I have not verified all of these claims. Finally, I appreciate the expansion made to the technical details of the algorithm in the Statement of Need section. That said, I now believe the last paragraph (L68-73) gets a little too far into the weeds. A sentence or two on the choice of basis would suffice.

I have checked the remaining three items on the checklist. The authors have addressed my comments and I recommend acceptance for this paper.

pdebuyl commented 3 years ago

Thank you very much for completing the review @jordanperr

@briandesilva sorry for the delay. I will now proceed to the remaining editorial steps https://joss.readthedocs.io/en/latest/editing.html#after-reviewers-recommend-acceptance

pdebuyl commented 3 years ago

@whedon check references

whedon commented 3 years ago

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1287/opre.43.4.684 is OK
- 10.1137/17M1162366 is OK
- 10.1109/JSEN.2018.2887044 is OK
- 10.1109/mcs.2018.2810460 is OK
- 10.1137/15M1036713 is OK
- 10.1017/jfm.2011.195 is OK
- 10.2514/6.2004-2415 is OK
- 10.1073/pnas.1517384113 is OK
- 10.1109/access.2018.2886528 is OK
- 10.1109/access.2020.3023625 is OK
- 10.1126/science.1165893 is OK
- 10.1103/physrevmaterials.2.083802 is OK
- 10.1111/j.2517-6161.1996.tb02080.x is OK
- 10.5281/zenodo.1173754 is OK
- 10.1364/oe.24.030433 is OK
- 10.1063/1.5066099 is OK
- 10.1063/1.4977057 is OK
- 10.1016/j.ymssp.2018.08.033 is OK
- 10.1126/sciadv.1602614 is OK
- 10.1098/rspa.2016.0446 is OK
- 10.1137/16m1086637 is OK
- 10.1137/18m116798x is OK
- 10.1017/jfm.2017.823 is OK
- 10.1063/1.5018409 is OK
- 10.1016/j.ifacol.2016.10.249 is OK
- 10.1103/physreve.96.023302 is OK
- 10.1016/j.jcp.2018.10.045 is OK
- 10.1098/rspa.2018.0335 is OK
- 10.1016/j.jcp.2019.07.049 is OK
- 10.1007/s00162-020-00536-w is OK
- 10.1103/physreve.101.010203 is OK
- 10.1115/1.4043148 is OK
- 10.1016/j.ocemod.2009.01.001 is OK
- 10.1109/cdc.2014.7040017 is OK
- 10.1017/jfm.2017.137 is OK
- 10.1073/pnas.1808909115 is OK
- 10.1016/j.jmsy.2018.01.011 is OK
- 10.1017/jfm.2018.147 is OK
- 10.1017/9781108380690 is OK
- 10.1007/s00162-020-00520-4 is OK
- 10.1002/cpa.20124 is OK
- 10.1016/0167-7152(84)90020-8 is OK
- 10.1016/j.crma.2004.08.006 is OK
- 10.1162/0899766053723032 is OK
- 10.1109/TIT.2006.871582 is OK
- 10.1109/tit.2006.885507 is OK
- 10.1109/tit.2005.862083 is OK
- 10.1109/MSP.2007.4286571 is OK
- 10.1109/tit.2009.2034811 is OK
- 10.1016/j.acha.2010.10.002 is OK
- 10.1145/1879141.1879192 is OK
- 10.1137/090766498 is OK
- 10.1111/j.1467-9868.2011.00783.x is OK
- 10.1137/110822724 is OK
- 10.1137/15M1019271 is OK
- 10.1109/tsipn.2016.2614903 is OK
- 10.1109/sam.2016.7569707 is OK
- 10.1137/16m1081270 is OK
- 10.2172/1405271 is OK

MISSING DOIs

- 10.1145/3384419.3430407 may be a valid DOI for title: A toolkit for spatial interpolation and sensor placement

INVALID DOIs

- None

pdebuyl commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pdebuyl commented 3 years ago

@briandesilva

Comments on the paper:

The year in Candès is not showing properly
The DOI recommendation by whedon for Narayanan seems correct, if so can you add it?

On the repo:

Could you add a warning that the netcdf data example is a 200MB download? Maybe also check if the file exists, to avoid re-downloading the data?
The examples run very well (no warning, nice results)!

You will need to archive your code on zenodo. https://joss.readthedocs.io/en/latest/submitting.html#the-review-process

Let me know about the doi and version of the archive.

jordanperr commented 3 years ago

One final comment @briandesilva - I think some of the mathematical notation around line 56, especially the subscripts, could be introduced more carefully. For example, I am not certain what the subscript y_i means. Since y is bold, it would appear to be a vector with length i, where i <= p is the number of "active" sensors. But then c_i represents the "action of a sensor," which implies you have separate ci for each sensor. Is C* a commonly used notation? Is it the same as s_* on line 65? It may be worthwhile to do one more pass of the mathematical notation to make sure each symbol is introduced and their meaning will be clear to broad audience.

briandesilva commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

briandesilva commented 3 years ago

@jordanperr, I've done another pass over the math symbols, adding more detail where necessary. Thanks for the suggestion, and please let me know if you think there is anything in the paper that needs to be clarified further.

@pdebuyl, I've addressed the issues you raised and registered PySensors with Zenodo. The DOI is 10.5281/zenodo.4312270 and the archived version is v0.3.2.

pdebuyl commented 3 years ago

Hi @briandesilva the doi for your archive is incorrect, it points to pysindy and not to PySensors

openjournals / joss-reviews