Closed editorialbot closed 11 months ago
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdf
Software report:
github.com/AlDanial/cloc v 1.88 T=0.13 s (467.3 files/s, 64596.1 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 32 942 2172 3802
Markdown 5 64 0 260
YAML 8 49 21 254
TeX 1 19 0 239
reStructuredText 10 67 52 128
TOML 1 11 3 69
INI 1 0 0 2
JSON 1 0 0 1
-------------------------------------------------------------------------------
SUM: 59 1152 2248 4755
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 1276
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1111/j.2517-6161.1996.tb02080.x is OK
- 10.1198/016214506000000735 is OK
- 10.1080/00401706.1967.10490502 is OK
- 10.1214/15-AOS1388 is OK
- 10.1287/opre.2015.1436 is OK
- 10.1111/j.1467-9868.2005.00532.x is OK
- 10.1080/10618600.2012.681250 is OK
- 10.1007/s00362-017-0882-z is OK
- 10.21105/joss.03024 is OK
- 10.1103/PhysRevB.106.144202 is OK
- 10.1103/PhysRevB.100.134108 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@htjb, @mhu48
This is the review thread. Firstly, type
@editorialbot generate my checklist
to generate your own checklist. In that checklist, there are many check items. Whenever you complete the corresponding task, you can check them off.
Please write your comments as separate posts and do not modify your checklist descriptions.
The review process is interactive so you can always interact with the authors, reviewers, and the editor. You can also create issues and pull requests in the target repository. Please do mention this thread's URL in the issues so we can keep tracking what is going on out of our world.
Please do not hesitate to ask me about anything at anytime.
Thank you in advance!
@mhu48 - Could you please generate your checklist before starting your review? Thank you in advance.
@lbluque this is a nice piece of code that I believe fills a gap in the capabilities of sklearn. The documentation is well written and formatted as is the paper.
I had a few comments on the code and paper below.
and
and are
.sparse-lm
being used in the literature? If so it might be good to reference these somewhere in the paper.sparse-lm
aims to be a catch all for the things missing in scikit-learn
so that the user only needs to install one package in addition to sklearn
. In the paper where you list the implemented regression models and model selection tools I think it would be good to add which packages currently feature these models e.g. (celer
and grupyr
) next to Lasso and Group Lasso. This will help to emphasis the benefits of having all of the models in one place and identify any gaps in the existing toolbox.nore
on line 45 should be nor
.GridSearchCV
functionality in sklearn
? You use the sklearn
grid search in the readme example.requirements.txt
. I'm aware that they are listed in the pyproject.toml
but this is harder to parse than a requirements.txt
.sparselm.model.RidgedBestSubsetSelection
under VARIABLES:
and a few of the other models. Similarly in GridSearchCV
.Thanks a lot, @htjb for the thorough review and comments! These are quite helpful, I'll start addressing them as soon as I can.
@lbluque - Could you please set the license of the software explicitly as defined in https://joss.readthedocs.io/en/latest/submitting.html. The license should be a in OSI-approved license
@lbluque - Could you please set the license of the software explicitly as defined in https://joss.readthedocs.io/en/latest/submitting.html. The license should be a in OSI-approved license
Hi @jbytecode. I am not sure what you mean by setting it explicitly. We have a BSD-3-clause-lbnl license, which is osi approved in our LICENSE file.
Can you please point out if I have missed anything?
@lbluque - Thank you, I mean the license type on the right side of the GitHub page, however, I have just realized the license link is linked to the license file that indicates it is licensed under BSD-3-clause.
Ah, got it! Unfortunately, I think Github can't parse our specific license to have the link mention 3-BSD explicitly.
@lbluque - That makes sense, thank you for the clarification.
I finally had some time to make edits. Thanks for your patience and your thorough review @htjb!
Paper
* I think in line 56 there is an unrendered symbol between `and` and `are`.
Fixed, thanks.
* Are you aware of any particular examples of `sparse-lm` being used in the literature? If so it might be good to reference these somewhere in the paper.
I am only aware of our own work. I have added some citations at the end of the end of the Statement of Need section (though it feels a bit like a self-plug :electric_plug: )
* You mention a few different packages that can be used to fit some specific sparse linear regression models. My understanding is that `sparse-lm` aims to be a catch all for the things missing in `scikit-learn` so that the user only needs to install one package in addition to `sklearn`. In the paper where you list the implemented regression models and model selection tools I think it would be good to add which packages currently feature these models e.g. (`celer` and `grupyr`) next to Lasso and Group Lasso. This will help to emphasis the benefits of having all of the models in one place and identify any gaps in the existing toolbox.
Great suggestion! I added a table in the usage section that makes it much more clear what is and isn't available in the different packages.
* I think it would be nice to see a physically motivated example/use case in the paper just to emphasis how important the tool is.
I personally prefer to exclude direct examples in JOSS papers to keep the manuscript short and concise. And especially for a package like
sparse-lm
with applicability in a very wide range of fields, I think it can also over-emphasize a specific field. In lieu, I added citations to many applications in various fields of regression with structured sparsity in the Summary and Statement of need sections. Please let me know your thoughts.* I think `nore` on line 45 should be `nor`.
Fixed thank you!
Code
* Could you please clarify for my benefit why you have reimplemented the `GridSearchCV` functionality in `sklearn`? You use the `sklearn` grid search in the readme example.
The only reason is that the
GridSearchCV
implemented insparse-lm
allows to automatically pick hyperparameters based on the "1-std rule" (1 std from the optimum, and those favor sparsity over CV error), rather thank the optimal value given by thesklearn
version, otherwise they are exactly the same.* It appears that the example in the readme crashes. This should ideally be fixed.
Fixed as well. Thanks a lot!
* There is a nice guide for contributing to the code in the documentation but it would be good to have this information in the README to since this is the first port of call for most people before looking at the documentation. I would just add a section saying contributions are welcome and link the documentation page to the readme.
Very good point. Added links to README to make it easier for users and devs.
* Similarly I would add install instructions to the README.
Same as above.
* I would add a list of requirements to the readme and maybe a `requirements.txt`. I'm aware that they are listed in the `pyproject.toml` but this is harder to parse than a `requirements.txt`.
Added a requirements.txt to the repo as well.
* The API documentation is very thorough but there are a few formatting issues that it would be good to check over. e.g for `sparselm.model.RidgedBestSubsetSelection` under `VARIABLES:` and a few of the other models. Similarly in `GridSearchCV`.
Thanks a lot for cathing these. I went in and think I have now fixed all these formatting issues.
Please have a look at these changes when you get a chance, and let us know if you have further comments or suggestions. Thanks again!
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hi @lbluque, the changes look good! The table is a nice addition to the paper and the readme is looking good.
I can understand your point about adding examples in to the paper. I think referencing different fields where sparse linear regression models are being used is a reasonable alternative and referencing a paper where the package has already been used in the statement of need is great.
The package is a nice addition to the python toolbox! @jbytecode I ticked off my checklist. Please let me know if there is anything else I need to do.
@lbluque neat work!
@htjb - Thank you for your review.
@mhu48 - Could you please update your status and inform us on how is your review going? Thank you in advance.
@mhu48 - Could you please update your status and inform us on how is your review going? Thank you in advance.
Thank you fo the reminder, apparently something interesting happened last week and I thought I already submitted the changes on github. Nevertheless, I have updated the list just now. Please let me know if it is still not reflected.
On a side note, how do I know how many papers I have reviewed for JOSS so far? I tried searching for email chains but that was quite a mess. Thanks so much for your help in advance.
Best,
Mign
On Sun, Nov 19, 2023 at 11:47 AM Mehmet Hakan Satman < @.***> wrote:
@mhu48 https://github.com/mhu48 - Could you please update your status and inform us on how is your review going? Thank you in advance.
— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/5867#issuecomment-1817929029, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKGGVY2LHAPHL2H5MSCMHYLYFJA45AVCNFSM6AAAAAA5CSXLOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXHEZDSMBSHE . You are receiving this because you were mentioned.Message ID: @.***>
@mhu48 - Do you have any suggestions/corrections/additions due to your review?
I think you can see a list of reviewed manuscripts by you in this page
Awesome, thanks very much!
On Sun, Nov 19, 2023 at 11:57 AM Mehmet Hakan Satman < @.***> wrote:
@mhu48 https://github.com/mhu48 - Here is the list of the reviewed papers by you: Link https://joss.theoj.org/papers/reviewed_by/mhu48
— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/5867#issuecomment-1817931399, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKGGVYYOJLM3V66B45DYPY3YFJCCHAVCNFSM6AAAAAA5CSXLOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXHEZTCMZZHE . You are receiving this because you were mentioned.Message ID: @.***>
@mhu48 - I can see you've already checked all of the tasks in your check list, however, it would be nice to have a review report of you here.
Hi,
Thank you for the reminder. I just saw your reply. Please find my report below.
"I'm pleased to report that the author has diligently addressed the concerns raised during the previous review process, and the changes made have significantly improved the overall quality of the package.
Specifically, the updated README file is now well-structured, providing clear and comprehensive documentation. The author has effectively incorporated the necessary information, making it accessible and user-friendly.
The concerns I previously raised and in the list of reviewer questions have been thoughtfully addressed, and the modifications implemented are not only acceptable but enhance the clarity and usability of the package. The inclusion of additional details has notably improved the overall robustness of the code.
In conclusion, this is a nice addition to the Python toolbox, offering valuable functionality for sparse linear regression models."
Best,
Ming
On Sun, Nov 19, 2023 at 12:02 PM Mehmet Hakan Satman < @.***> wrote:
@mhu48 https://github.com/mhu48 - I can see you've check all of the tasks in your check list, however, it would be nice to have a review report of you here.
— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/5867#issuecomment-1817932656, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKGGVY5GF7LJGTUT2L2EY6DYFJCUVAVCNFSM6AAAAAA5CSXLOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXHEZTENRVGY . You are receiving this because you were mentioned.Message ID: @.***>
@editorialbot check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1111/j.2517-6161.1996.tb02080.x is OK
- 10.1198/016214506000000735 is OK
- 10.1080/00401706.1967.10490502 is OK
- 10.1214/15-AOS1388 is OK
- 10.1287/opre.2015.1436 is OK
- 10.1111/j.1467-9868.2005.00532.x is OK
- 10.1080/10618600.2012.681250 is OK
- 10.1007/s00362-017-0882-z is OK
- 10.21105/joss.03024 is OK
- 10.1257/jep.31.2.3 is OK
- 10.23919/CCC52363.2021.9549471 is OK
- 10.1080/03610918.2011.611311 is OK
- 10.1039/C7RE00210F is OK
- 10.1186/1471-2105-8-60 is OK
- 10.1103/PhysRevB.100.134108 is OK
- 10.1016/j.commatsci.2022.112000 is OK
- 10.1103/PhysRevB.106.024203 is OK
- 10.1103/PRXEnergy.2.043005 is OK
- 10.1103/PhysRevB.106.144202 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot set <DOI here> as archive
@editorialbot set <version here> as version
@editorialbot generate pdf
@editorialbot check references
and ask author(s) to update as needed@editorialbot recommend-accept
@lbluque - I've just sent a pull request that includes some minor changes in both the bibtex and the manuscript. Could you please review the changes and then merge if it is okay to you.
Note: In equation 1, after the squre root, number of pipes at the start does not match the ones at the end ($\sqrt{|\mathbf{g}}||\beta_{\mathbf{g}}||_2 $). Could you please review the equation 1 and correct the notation?
Please ping me when you've done with it. Thank you in advance.
$$ \begin{equation} \beta^* = \underset{\beta}{\text{argmin}}\;||\mathbf{X} \beta - \mathbf{y}||^22 + (1-\alpha)\lambda\sum{\mathbf{g}\in G}\sqrt{|\mathbf{g} }||\beta_{\mathbf{g}}||_2 + \alpha\lambda||\beta||_1 \end{equation} $$
@editorialbot check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1111/j.2517-6161.1996.tb02080.x is OK
- 10.1198/016214506000000735 is OK
- 10.1080/00401706.1967.10490502 is OK
- 10.1214/15-AOS1388 is OK
- 10.1287/opre.2015.1436 is OK
- 10.1111/j.1467-9868.2005.00532.x is OK
- 10.1080/10618600.2012.681250 is OK
- 10.1007/s00362-017-0882-z is OK
- 10.21105/joss.03024 is OK
- 10.1257/jep.31.2.3 is OK
- 10.23919/CCC52363.2021.9549471 is OK
- 10.1080/03610918.2011.611311 is OK
- 10.1039/C7RE00210F is OK
- 10.1186/1471-2105-8-60 is OK
- 10.1103/PhysRevB.100.134108 is OK
- 10.1016/j.commatsci.2022.112000 is OK
- 10.1103/PhysRevB.106.024203 is OK
- 10.1103/PRXEnergy.2.043005 is OK
- 10.1103/PhysRevB.106.144202 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@lbluque - I've just sent a pull request that includes some minor changes in both the bibtex and the manuscript. Could you please review the changes and then merge if it is okay to you.
Note: In equation 1, after the squre root, number of pipes at the start does not match the ones at the end (|g||βg||2). Could you please review the equation 1 and correct the notation?
Please ping me when you've done with it. Thank you in advance.
β∗=argminβ;||Xβ−y||22+(1−α)λ∑g∈G|g||βg||2+αλ||β||1
Thanks a for the detailed checks @jbytecode! I fixed the above and pushed a few edits to the paper. Please have a look when you get a chance.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@lbluque - the paper looks better now, thank you.
Could you please perform these tasks:
vx.y.z
, e.g., v1.0.3
. Meanwhile you are fulfilling the requirements, I will be taking a look at the paper again.
Thank you in advance.
Hi @jbytecode - here is the release and zenodo archive information:
@editorialbot set v0.5.2 as version
Done! version is now v0.5.2
@lbluque - Thank you, I've just registered the version id. Could you please add ORCIDs of authors in the archive and then ping me again?
Thanks @jbytecode - added orcids!
@editorialbot set 10.5281/zenodo.10246640 as archive
Submitting author: !--author-handle-->@lbluque<!--end-author-handle-- (Luis Barroso-Luque) Repository: https://github.com/CederGroupHub/sparse-lm Branch with paper.md (empty if default branch): paper Version: v0.5.2 Editor: !--editor-->@jbytecode<!--end-editor-- Reviewers: @htjb, @mhu48 Archive: 10.5281/zenodo.10246640
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
@htjb & @mhu48, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jbytecode 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 ✨
Checklists
📝 Checklist for @htjb
📝 Checklist for @mhu48