openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
720 stars 38 forks source link

[REVIEW]: nnTensor: An R package for non-negative matrix/tensor decomposition #5015

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@kokitsuyuzaki<!--end-author-handle-- (Koki Tsuyuzaki) Repository: https://github.com/rikenbit/nnTensor Branch with paper.md (empty if default branch): Version: v1.1.14 Editor: !--editor-->@prashjha<!--end-editor-- Reviewers: @Stat-Cook, @oneilsh Archive: 10.5281/zenodo.7848088

Status

status

Status badge code:

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

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

@Stat-Cook & @oneilsh, 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:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @prashjha 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 @Stat-Cook

📝 Checklist for @oneilsh

editorialbot commented 1 year 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
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.03 s (1455.7 files/s, 153125.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               42            344            323           4068
Markdown                         3             29              0            145
TeX                              1              7              0             56
YAML                             1             12             13             45
Dockerfile                       1              1              2              4
-------------------------------------------------------------------------------
SUM:                            48            393            338           4318
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 603

editorialbot commented 1 year ago

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

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.1109/acssc.2016.7869679 may be a valid DOI for title: Tensorlab 3.0 - Numerical optimization strategies for large-scale constrained and coupled matrix/tensor factorization
- 10.1371/journal.pone.0217994 may be a valid DOI for title: Fast Optimization of Non-Negative Matrix Tri-Factorization
- 10.1142/9789812776136_0027 may be a valid DOI for title: Extracting Gene Expression Profiles Common to Colon and Pancreatic Adenocarcinoma using Simultaneous nonnegative matrix factorization
- Errored finding suggestions for "A non-negative matrix factorization method for det...", please try later

INVALID DOIs

- None
Stat-Cook commented 1 year ago

Review checklist for @Stat-Cook

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Stat-Cook commented 1 year ago

@kokitsuyuzaki can I confirm how you intend the paper to be viewed? When I visit paper.md there are several unprocessed tags, either an error in the github markdown or I'm not viewing as intended.

prashjha commented 1 year ago

@editorialbot generate pdf

prashjha commented 1 year ago

Dear @Stat-Cook and @oneilsh, please read the first couple of comments in this thread and create your review checklist if not done already. You can read the reviewer guidelines here. Also, you can browse the closed "REVIEW" issues on the "joss-reviews" repository to get some ideas on how to complete the reviews. Good luck!

editorialbot commented 1 year ago

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

Stat-Cook commented 1 year ago

@kokitsuyuzaki you have both a license file and a license in the read me. Is this intentional, and if not please reduce for clarity.

kokitsuyuzaki commented 1 year ago

@kokitsuyuzaki can I confirm how you intend the paper to be viewed? When I visit paper.md there are several unprocessed tags, either an error in the github markdown or I'm not viewing as intended.

@Stat-Cook It might be because of the difference between the markdown of JOSS and that of GitHub. To review my manuscript, please check the latest PDF generated by @editorialbot generate pdf. cf. https://github.com/openjournals/joss-reviews/issues/5015#issuecomment-1353379430

kokitsuyuzaki commented 1 year ago

@kokitsuyuzaki you have both a license file and a license in the read me. Is this intentional, and if not please reduce for clarity.

@Stat-Cook Sorry, I'll remove the licence section in README.md later.

kokitsuyuzaki commented 1 year ago

@Stat-Cook l've removed the license section in README.md. https://github.com/rikenbit/nnTensor

kokitsuyuzaki commented 1 year ago

@Stat-Cook In my environment, plotTensor3D works properly. Could you give me some hint to reproduce the error (e.g., sessionInfo())? https://github.com/rikenbit/nnTensor/issues/1

kokitsuyuzaki commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

kokitsuyuzaki commented 1 year ago

As whedon mentioned it, I added DOIs for the references as possible. https://github.com/rikenbit/nnTensor/blob/master/paper/paper.bib

kokitsuyuzaki commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

oneilsh commented 1 year ago

Review checklist for @oneilsh

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

oneilsh commented 1 year ago

@kokitsuyuzaki I've started taking a look at nnTensor; impressive work! My initial comments are around some of the documentation and example usage. The help for the functions (I'm looking at the doc for NTD, others look similar) seems to assume that the num_modes of the input tensor will be 3 (with explicit references to I1, I2, I3, J1, J2, and J3), but I see that the methods are implemented generally and work with tensors with more modes. I think the help docs should reflect the general case, especially the argument definitions and return value definitions. You could then add a Details section that provides more in-depth explanations on specific tensor shapes if it helps. The example usage is also quite sparse, and would benefit from examples on different input and core tensor shapes and using/interpreting the output.

prashjha commented 1 year ago

Hello @kokitsuyuzaki, could you please take care of the recent comments from @oneilsh?

prashjha commented 1 year ago

Hi @Stat-Cook, how is the review progressing?

kokitsuyuzaki commented 1 year ago

@prashjha Sorry, I was temporarily too busy to work on this review. To answer @oneilsh 's point, I am currently working on vignettes for nnTensor.

kokitsuyuzaki commented 1 year ago

@Stat-Cook Sorry for the late reply. A bug regarding plotTensor3D has been resolved. For more information, please see the comment in the following Issue. https://github.com/rikenbit/nnTensor/issues/1

kokitsuyuzaki commented 1 year ago

@oneilsh Sorry for the late reply.

I think the help docs should reflect the general case, especially the argument definitions and return value definitions.

Thank you for your suggestion. I have updated the manuals to explain that any order tensor can be specified as the input. https://github.com/rikenbit/nnTensor/blob/master/man/NTD.Rd#L30 https://github.com/rikenbit/nnTensor/blob/master/man/NTF.Rd#L25 https://github.com/rikenbit/nnTensor/blob/master/man/recTensor.Rd#L20

You could then add a Details section that provides more in-depth explanations on specific tensor shapes if it helps. The example usage is also quite sparse, and would benefit from examples on different input and core tensor shapes and using/interpreting the output.

I have created three vignettes to explain the way to use NMF, NMTF, siNMF, jNMF, NTF, and NTD. https://github.com/rikenbit/nnTensor/tree/master/vignettes https://cran.r-project.org/web/packages/nnTensor/vignettes/nnTensor-1.html https://cran.r-project.org/web/packages/nnTensor/vignettes/nnTensor-2.html https://cran.r-project.org/web/packages/nnTensor/vignettes/nnTensor-3.html

These vignettes are designed for a single matrix, multiple matrices, and a single tensor, respectively, and the third vignette explains that NTD can also work as NTD-2/NTD-1 which extract factor matrices only from specific modes. In the third vignette, I also explain that NTF and NTD can be applied to higher-order tensors.

kokitsuyuzaki commented 1 year ago

@prashjha I have responded to all the comments of two reviewers as above.

prashjha commented 1 year ago

Hi, @Stat-Cook and @oneilsh, please let me know how the review is progressing. Thanks, @kokitsuyuzaki, for addressing the comments.

oneilsh commented 1 year ago

Thank you for the ping @prashjha, I'll get back to it this week.

oneilsh commented 1 year ago

@kokitsuyuzaki the vignettes look great and even provide multiple examples. The only remaining issue I see is the lack of community guidelines mentioned in the checklist. A CONTRIBUTING.md file covering those points would do.

@prashjha, once that detail is fixed my review is done. @kokitsuyuzaki, thank you for developing a useful package!

kokitsuyuzaki commented 1 year ago

Thanks, @oneilsh. I created my first CONTRIBUTING.md and learned a lot. Please check the following markdowns. https://github.com/rikenbit/nnTensor#contributing https://github.com/rikenbit/nnTensor/blob/master/CONTRIBUTING.md https://github.com/rikenbit/nnTensor/blob/master/.github/ISSUE_TEMPLATE/bug_report.md https://github.com/rikenbit/nnTensor/blob/master/CODE_OF_CONDUCT.md

kokitsuyuzaki commented 1 year ago

@prashjha I have responded to @oneilsh's comment. There is no change in the manuscript. I just put some markdown files on the repository.

prashjha commented 1 year ago

@oneilsh, thanks for finishing the review. Just to confirm, you recommend accepting the submission, right?

oneilsh commented 1 year ago

@prashjha correct, I recommend acceptance.

prashjha commented 1 year ago

@oneilsh, great

kokitsuyuzaki commented 1 year ago

@Stat-Cook I have responded to your review. Could you please check it? https://github.com/openjournals/joss-reviews/issues/5015#issuecomment-1431484433

Stat-Cook commented 1 year ago

@kokitsuyuzaki apologies - a few spells of sickness have limited my time. I'll be looking at this in the next few days.

Stat-Cook commented 1 year ago

The recommendations from oneilsh covered some of my earlier concerns. I'm happy the package does what it says on the tin - there's one recommendation I've added to the issues tracker for consideration but it doesn't effect functionality. Happy to recommend acceptance.

kokitsuyuzaki commented 1 year ago

@Stat-Cook Thank you for checking the details. The vignette has been fixed in v1.1.13. https://github.com/rikenbit/nnTensor/issues/2

kokitsuyuzaki commented 1 year ago

@Stat-Cook @oneilsh Thank you for your cooperation in the peer review process. Thanks to you guys, we have better documents and GitHub configuration.

kokitsuyuzaki commented 1 year ago

@prashjha Two reviewers recommended acceptance. Is there anything I can do next?

prashjha commented 1 year ago

@Stat-Cook, thank you for the review and for providing your decision.

@kokitsuyuzaki, I will go over the draft one last time before handing your submission to Editor in Chief for final decision.

prashjha commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

prashjha commented 1 year ago

Hi @kokitsuyuzaki, your draft looks good. Could you archive (if not done already) the release using zenodo and provide the archive reference so that I can associate it with your JOSS submission? Make sure that the zenodo archive's title matches this JOSS submission's title.

Once you are done, I will hand your paper to EiC for the final decision.

kokitsuyuzaki commented 1 year ago

Thanks, @prashjha I archived the release and the Zenodo DOI is 10.5281/zenodo.7839285 Please check the links below.

https://zenodo.org/record/7839285#.ZD32Hy33JhE https://github.com/rikenbit/nnTensor/releases/tag/joss_released

prashjha commented 1 year ago

Hi @kokitsuyuzaki, could you please attach the latest version (formatted like vx.x.x or x.x.x) to the Zenodo archive? I see the latest version attached to your zenoda archive as 'joss_released'. This is a nonstandard version name. Once you update zenodo, let me know the archive DOI and also the corresponding released version of your repository so that I can associate these details with your JOSS article. Thanks.

kokitsuyuzaki commented 1 year ago

Sorry, I've created new tag v1.1.14 and the Zenodo DOI is 10.5281/zenodo.7848088.

https://zenodo.org/record/7848088#.ZEDeni33JhE https://github.com/rikenbit/nnTensor/releases/tag/v1.1.14

prashjha commented 1 year ago

@editorialbot set v1.1.14 as version