[REVIEW]: SISSO++: A C++ Implementation of the Sure Independence Screening and Sparisifying Operator Approach

whedon commented 2 years ago

Submitting author: !--author-handle-->@tpurcell90@jarvist<!--end-editor-- Reviewers: @ml-evs, @AstroBarker Archive: 10.5281/zenodo.6245396

: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/72b4c8d40d9a0401039ade39a7801759"><img src="https://joss.theoj.org/papers/72b4c8d40d9a0401039ade39a7801759/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/72b4c8d40d9a0401039ade39a7801759/status.svg)](https://joss.theoj.org/papers/72b4c8d40d9a0401039ade39a7801759)

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

@ml-evs & @AstroBarker, 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 @jarvist 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 @ml-evs

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

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 (@tpurcell90) 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: Does the paper have a section titled 'Statement of Need' that clearly states 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 @AstroBarker

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

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 (@tpurcell90) 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: Does the paper have a section titled 'Statement of Need' that clearly states 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 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @ml-evs, @AstroBarker 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 2 years ago

Wordcount for paper.md is 756

whedon commented 2 years ago

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

OK DOIs

- 10.1021/acscatal.8b04478 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1088/2515-7639/ab077b is OK
- 10.1103/PhysRevMaterials.2.083802 is OK
- 10.1557/mrc.2019.85 is OK
- 10.1016/j.cej.2019.123412 is OK
- 10.1126/sciadv.aay2631 is OK
- 10.1038/s41467-018-06682-4 is OK
- 10.1021/acsami.9b14530 is OK
- 10.1038/s41467-021-22048-9 is OK
- 10.1021/acs.accounts.1c00153 is OK
- 10.1021/acscatal.0c04170 is OK
- 10.1021/acs.jcim.9b00807 is OK
- 10.1126/sciadv.aav0693 is OK
- 10.1103/PhysRevMaterials.4.034204 is OK

MISSING DOIs

- None

INVALID DOIs

- None

whedon commented 2 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.37 s (1234.1 files/s, 207552.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                            150           4975           3027          29146
C/C++ Header                    81           2657          13448           7235
Python                         125           1926           1948           6285
Markdown                        13            350              0           1958
CMake                           16            245            627           1403
JSON                            21              0              0            398
reStructuredText                42            249            548            334
TeX                              1              2              0            289
YAML                             1             15              0            199
TOML                             1             11              1             51
MATLAB                           3             12             23             44
Dockerfile                       1             12              2             33
DOS Batch                        1              8              1             26
Bourne Shell                     4              1              0             24
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                           461          10467          19632          47434
-------------------------------------------------------------------------------

Statistical information for the repository 'e00fe9d2d46ac614eb10e261' was
gathered on 2021/11/30.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Thomas                         390         80945          45435           55.45
Thomas Purcell                 329         72097          29427           44.55

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Thomas                    70728           87.4          7.3               25.73
Thomas Purcell                8            0.0          7.8                0.00

whedon commented 2 years ago

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

jarvist commented 2 years ago

Thanks for reviewing @AstroBarker and @ml-evs!

Please beware accidentally editing each other's checklists. (I do this all the time.)

What sort of timeline would be acceptable for the first round of review? I think it would be good to get back to the authors before Christmas / New Year closures if possible.

ml-evs commented 2 years ago

What sort of timeline would be acceptable for the first round of review? I think it would be good to get back to the authors before Christmas / New Year closures if possible.

This should be do-able! I will chunk my review into comments on the paper, repo/docs and then the code/usage to minimise any hold ups.

I have had a look at the paper and skimmed relevant parts of the documentation, and have prepared some comments for @tpurcell90 below.

Paper review

The software paper nicely introduces the exisiting applications of the SISSO approach (primarily in materials science) and the benefits brought by the SISSO++ implementation. I have split my suggestions below into what I would consider blocking/non-blocking for publication. The paper could do with another round of proofreading and some sentences could be clarified. I have attached a separate list of proofreading suggestions of things I have spotted below.

Suggestions

Blocking

[ ] Without relitigating exisiting SISSO papers, I think 1 sentence layman summaries for symbolic regression, compressed sensing and sure-independence screening would add to the paper.
[ ] Other dependencies of the library should be cited if they are core components, from the deps list it looks like COIN-OR, NLopt, seaborn, scipy, numpy, pandas, and maybe even Boost, OpenMP, MPI, Python... are all valid targets.
[ ] A brief description of the required user inputs would be good: does the user provide a dataframe of precomputed descriptors for all the data? How does training and e.g., cross-validation work? If these general ML workflows are not implemented directly within SISSO++, perhaps expand on the section about using the Python bindings within existing computational workflows to emphasize that the package should be used within such ML workflows.
[ ] Although it is redundant given the publication venue, could you mention that SISSO++ is open source and its license in the paper?
[ ] Discuss differences/synergies with other packages like https://github.com/rouyang2017/SISSO, https://github.com/Matgenix/pysisso, https://github.com/chuanqixu/SISSOkit, https://github.com/NREL/SISSORegressor_MATLAB. Depending on the author's familiarity of these packages and the maturity/completeness, this could instead be provided in the documentation.
[ ] A sentence on typical problem scale could be useful: trillions of descriptors is a lot, how does the implementation scale with number of data points, number of input descriptors, number of basis functions and number of concurrent processes? Are there heavy disk/memory requirements? Are intermediate results cached/checkpointed? What problems will require the use of massively parallel compute as discussed? Much of this could also be moved to the documentation, but a rough idea in the paper would be helpful if any of the answers to the above are extreme (e.g., if you need 1 TB of RAM to even run the examples)

Non-blocking

[ ] There are some nice figures including the tutorial documentation online, it could be nice to include one here to showcase the post-processing capabilities of this package.
[ ] A constrast with other cutting edge algorithms would be useful: interpretability benefit vs neural networks, data efficiency and inference speeds vs graph neural networks, improved accuracy vs classical ML (RF etc.)? For materials science in particular, it could be useful to contrast with AutoML approaches like https://github.com/hackingmaterials/automatminer and possible syneries with the underlying https://github.com/hackingmaterials/matminer descriptor library. The descriptions of e.g., symbolic regression from my first suggestion may already address this.
[ ] Given the materials science focus of the applications mentioned, although it is probably out of scope for the software implementation paper, I would (personally) love to see benchmarks of SISSO on e.g., MatBench which could show the relative strength of SISSO vs other methods. If these have been performed/published elsewhere, it could be nice to reference them.

Proofreading

[ ] 12: "so called" -> "so-called".
[ ] 12: Could you perhaps reword this setence clarify the difference between the "identified" analytic functions, descriptors and what might be called "basis functions" (from user input)?
[ ] 10: citation for compressed sensing and symbolic regression
[ ] 17: This final sentence is not clear to me, "best" is vague, perhaps it could be rephrased
[ ] 19: Because is reused a lot in this paragraph
[ ] 31: Format "python" as "Python" everywhere, likewise "openMP" -> "OpenMP".
[ ] 38: "massively parallel environment data production" is missing a word; is this step really "data production" or is "feature space engineering" a better fit?
[ ] 39: "computationally intensive" not "computational intensive"
[ ] 43: Missing full stop after sklearn citation.
[ ] 45: "new-users" -> "new users".
[ ] 52: This bulletpoint should be revised (and this is where the NLOpt etc. citations could be included)
[ ] 54: "Features with better defined non-linearities of the models" does not make sense to me. The citation of unpublished/missing is probably fine, but a preprint would be even better... I will try to remember to check this again at the final stages.
[ ] 57: Missing word, presumably 'API documentation'

tpurcell90 commented 2 years ago

@ml-evs Thank you for your comments. I updated the paper in the joss_review branch.

I addressed all of the blocking comments and proofreading comments, except the typical problem scale which I will do in the documentation. I will also probably do another round of proofreading at the end of the submission process.

For the non-blocking comments, I agree it would be interesting/good to include these points. However, I think they are out of scope for this style of publication and would put be over the 1,000 word cap.

whedon commented 2 years ago

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

tpurcell90 commented 2 years ago

@whedon commands

whedon commented 2 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

tpurcell90 commented 2 years ago

@whedon generate pdf from branch joss_review

whedon commented 2 years ago

Attempting PDF compilation from custom branch joss_review. Reticulating splines etc...

whedon commented 2 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 2 years ago

:wave: @AstroBarker, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @ml-evs, please update us on how your review is going (this is an automated reminder).

ml-evs commented 2 years ago

@ml-evs Thank you for your comments. I updated the paper in the joss_review branch.

I addressed all of the blocking comments and proofreading comments, except the typical problem scale which I will do in the documentation. I will also probably do another round of proofreading at the end of the submission process.

For the non-blocking comments, I agree it would be interesting/good to include these points. However, I think they are out of scope for this style of publication and would put be over the 1,000 word cap.

Thanks for that @tpurcell90 - I'm fine with the above. I will take another look at the paper once you have finished the proofreading, and will move my attention to testing the code and going through the repo. I will write my comments here, do you want me to also raise issues on GitLab directly?

tpurcell90 commented 2 years ago

@ml-evs Thank you for your comments. I updated the paper in the joss_review branch. I addressed all of the blocking comments and proofreading comments, except the typical problem scale which I will do in the documentation. I will also probably do another round of proofreading at the end of the submission process. For the non-blocking comments, I agree it would be interesting/good to include these points. However, I think they are out of scope for this style of publication and would put be over the 1,000 word cap.

Thanks for that @tpurcell90 - I'm fine with the above. I will take another look at the paper once you have finished the proofreading, and will move my attention to testing the code and going through the repo. I will write my comments here, do you want me to also raise issues on GitLab directly?

Thanks!

And if you don't mind posting issues on gitlab that would also be good. It's nice when those types of changes can all be logged in the same repo.

ml-evs commented 2 years ago

And if you don't mind posting issues on gitlab that would also be good. It's nice when those types of changes can all be logged in the same repo.

I have started raising a few issues on the documentation at https://gitlab.com/sissopp_developers/sissopp, and I will keep this comment up to date with the check-list:

[x] rendering of docs #2
[x] suggestions for installation instructions #3
[x] finalising docker container
[x] question re: Python CV implementation
[x] typos in paper and tutorials
[x] fixes for the tutorials and documentation

jarvist commented 2 years ago

Wonderful work everyone! And thanks for the reminder @whedon.

tpurcell90 commented 2 years ago

I opened up two MRs to address both the issues above. We can discuss them there or here.

AstroBarker commented 2 years ago

Sorry for the delay getting back on this! I have read over the new draft of the paper, and all of my comments from the previous version have been addressed already! As someone who does not have domain experience with these techniques, my opinion is that the paper sufficiently addressed the need for this software and demonstrated its prior uses in the literature. I am still reading through the documentation and will open issues on gitlab once I have finished with that.

jarvist commented 2 years ago

Thank you @AstroBarker ! Are you finished with review & happy with any edits?

@ml-evs , are there still unresolved issues? (I see in the comments that some edits have been made, but your review checkboxes for documentation & paper are not yet checked.)

@tpurcell90 Do you still have outstanding edits?

ml-evs commented 2 years ago

Thank you @AstroBarker ! Are you finished with review & happy with any edits?

@ml-evs , are there still unresolved issues? (I see in the comments that some edits have been made, but your review checkboxes for documentation & paper are not yet checked.)

@tpurcell90 Do you still have outstanding edits?

Hi @jarvist, there's a couple of open issues/PRs on the gitlab repo (I have not updated my comment above for a while) but I think we are nearly there.

tpurcell90 commented 2 years ago

Hi @jarvist I am in the process of finishing up a sklearn interface to address some of the final comments of @ml-evs. I am right now writing a first draft of the tutorial for that so it should be done relatively soon.

jarvist commented 2 years ago

Dear @tpurcell90 , how are you coming along?

ml-evs commented 2 years ago

@jarvist: I think I can probably answer on @tpurcell90's behalf as the ball is currently in my court, I am just doing some final testing of the mega PR at https://gitlab.com/sissopp_developers/sissopp/-/merge_requests/35 that incorporates all of our review suggestions.

ml-evs commented 2 years ago

@whedon generate pdf from branch joss_review

whedon commented 2 years ago

Attempting PDF compilation from custom branch joss_review. Reticulating splines etc...

whedon commented 2 years ago

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

tpurcell90 commented 2 years ago

@whedon generate pdf from branch joss_review

whedon commented 2 years ago

Attempting PDF compilation from custom branch joss_review. Reticulating splines etc...

whedon commented 2 years ago

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

tpurcell90 commented 2 years ago

@whedon generate pdf from branch joss_review

whedon commented 2 years ago

Attempting PDF compilation from custom branch joss_review. Reticulating splines etc...

whedon commented 2 years ago

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

ml-evs commented 2 years ago

I have now completed my review and recommend accepting this paper.

The SISSO++ package provides a useful, usable and modern interface to an interesting approach to symbolic regression that has so far primarily been used in the materials science domain. This particular method is difficult to implement efficiently; the C++/MPI + Python binding approach here adds a lot of value and scalability to larger machines. The code is fully documented and well-tested, and the tutorials are very thorough, guiding the user through a published research problem.

cc @jarvist

ml-evs commented 2 years ago

Note, there was quite a lot of back-and-forth on the GitLab repo (hence the radio silence here). As previously linked, all of the recommended changes were collected into one big PR: https://gitlab.com/sissopp_developers/sissopp/-/merge_requests/35 with links out to the relevant sub-issues/PRs.

tpurcell90 commented 2 years ago

@jarvist do we need to do anything else for this review?

tpurcell90 commented 2 years ago

@whedon generate pdf from master

editorialbot commented 2 years ago

My name is now @editorialbot

tpurcell90 commented 2 years ago

@editorialbot generate pdf from master

tpurcell90 commented 2 years ago

@editorialbot commands

danielskatz commented 2 years ago

@tpurcell90 - sorry, we're in the midst of a change of bot - if you can wait a few hours, please do

arfon commented 2 years ago

@editorialbot commands

editorialbot commented 2 years ago

Hello @arfon, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Add to this issue's reviewers list
@editorialbot add @username as reviewer

# Remove from this issue's reviewers list
@editorialbot remove @username from reviewers

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Assign a user as the editor of this submission
@editorialbot assign @username as editor

# Remove the editor assigned to this submission
@editorialbot remove editor

# Remind an author or reviewer to return to a review after a 
# certain period of time (supported units days and weeks)
@editorialbot remind @reviewer in 2 weeks

# Check the references of the paper for missing DOIs
# Optionally, it can be run on a non-default branch 
@editorialbot check references
@editorialbot check references from custom-branch-name

# Perform checks on the repository
# Optionally, it can be run on a non-default branch 
@editorialbot check repository
@editorialbot check repository from custom-branch-name

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for version
@editorialbot set v1.0.0 as version

# Set a value for archive
@editorialbot set 10.21105/zenodo.12345 as archive

# Set a value for branch
@editorialbot set joss-paper as branch

# Reject paper
@editorialbot reject

# Withdraw paper
@editorialbot withdraw

# Invite an editor to edit a submission (sending them an email)
@editorialbot invite @(.*) as editor

# Generates the pdf paper
@editorialbot generate pdf

# Recommends the submission for acceptance
@editorialbot recommend-accept

# Accept and publish the paper
@editorialbot accept

# Flag submission with questionable scope
@editorialbot query scope

# Get a link to the complete list of reviewers
@editorialbot list reviewers

# Open the review issue
@editorialbot start review

arfon commented 2 years ago

@tpurcell90 – apologies. You should be good to go again now.

tpurcell90 commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

Attempting PDF compilation. Reticulating splines etc...

editorialbot commented 2 years ago

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

openjournals / joss-reviews