openjournals / joss-reviews

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

[REVIEW]: cca-zoo #3823

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: @jameschapman19 (James Chapman) Repository: https://github.com/jameschapman19/cca_zoo Version: v1.10.8 Editor: @emdupre Reviewer: @robbisg, @hugorichard, @ejolly Archive: 10.5281/zenodo.5786616

: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

Status badge code:

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

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

@robbisg & @hugorichard & @ejolly, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. 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 @emdupre 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 @robbisg

✨ 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

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @hugorichard

✨ 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

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @ejolly

✨ 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

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @robbisg, @hugorichard, @ejolly 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:

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

watching

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

Wordcount for paper.md is 1397

whedon commented 3 years ago
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.14 s (436.4 files/s, 106033.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          36            779           1686           4621
Jupyter Notebook                 3              0           4301           2375
reStructuredText                14            216            139            255
TeX                              1             26              0            225
Markdown                         2             47              0            216
YAML                             4             17             17             91
DOS Batch                        1              8              1             26
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            62           1097           6151           7818
-------------------------------------------------------------------------------

Statistical information for the repository '701fd338069e8ec2384f9b55' was
gathered on 2021/10/13.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Hao-Ting Wang                    2            82             25            0.20
James Chapman                  423         17935          13864           58.33
jameschapman                    55         12783           9824           41.47

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
Hao-Ting Wang                48           58.5          4.1                6.25
James Chapman              7038           39.2          2.5                7.22
whedon commented 3 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.2307/2333955 may be a valid DOI for title: Relations between two sets of variates
- 10.1016/0304-4076(76)90010-5 may be a valid DOI for title: Canonical ridge and econometrics of joint production
- 10.1162/0899766042321814 may be a valid DOI for title: Canonical correlation analysis: An overview with application to learning methods
- 10.1007/s11336-011-9206-8 may be a valid DOI for title: Regularized generalized canonical correlation analysis
- 10.1093/biostatistics/kxp008 may be a valid DOI for title: A penalized matrix decomposition, with applications to sparse principal components and canonical correlation analysis
- 10.2202/1544-6115.1329 may be a valid DOI for title: Quantifying the association between gene expressions and DNA-markers by penalized canonical correlation analysis
- 10.2202/1544-6115.1406 may be a valid DOI for title: Sparse canonical correlation analysis with application to genomic data integration
- 10.1111/biom.13043 may be a valid DOI for title: An iterative penalized least squares approach to sparse canonical correlation analysis
- 10.3389/fninf.2016.00049 may be a valid DOI for title: Pyrcca: regularized kernel canonical correlation analysis in python and its applications to neuroimaging
- 10.1007/978-1-4612-4228-4_3 may be a valid DOI for title: The canonical correlations of matrix pairs and their numerical computation
- 10.1109/allerton.2015.7447071 may be a valid DOI for title: Stochastic optimization for deep CCA via nonlinear orthogonal iterations
- 10.1109/cvpr.2007.383137 may be a valid DOI for title: Tensor canonical correlation analysis for action classification
- 10.1109/tnn.2007.891186 may be a valid DOI for title: Variational Bayesian approach to canonical correlation analysis

INVALID DOIs

- None
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:

emdupre commented 3 years ago

Hi everyone ! :wave: Thanks again for agreeing to review this submission ! The review will take place in this issue.

Whenever possible, please open relevant issues on the linked software repository (and cross-link them with this issue) rather than discussing them here. This helps to make sure that feedback is translated into actionable items to improve the software. If you aren't sure how to get started, please see the Reviewing for JOSS guide -- and, of course, feel free to ping me with any questions !

@jameschapman19, one small formatting note : It looks like your paper.bib file lacks DOI fields for the included entries. This is causing whedon to complain about missing DOIs. Can you please add a DOI field for each entry ? You can see a first guess for the relevant DOIs -- to be confirmed for correctness ! -- in whedon's comment here.

jameschapman19 commented 3 years ago

Hi @emdupre. I have updated the DOIs for the ones that @whedon flagged and checked them. I guess the ones that don't flag are OK? The JOSS template bib doesn't have all references with DOIs.

emdupre 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.2307/2333955 is OK
- 10.1016/0304-4076(76)90010-5 is OK
- 10.1162/0899766042321814 is OK
- 10.1007/s11336-011-9206-8 is OK
- 10.1093/biostatistics/kxp008 is OK
- 10.2202/1544-6115.1329 is OK
- 10.2202/1544-6115.1406 is OK
- 10.1111/biom.13043 is OK
- 10.3389/fninf.2016.00049 is OK
- 10.1007/978-1-4612-4228-4_3 is OK
- 10.1109/allerton.2015.7447071 is OK
- 10.1109/cvpr.2007.383137 is OK
- 10.1109/tnn.2007.891186 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 years ago

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

whedon commented 3 years ago

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

whedon commented 3 years ago

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

robbisg commented 3 years ago

Sorry for the delay, I will review the package this week.

hugorichard commented 3 years ago

@emdupre, here is my review:

This is a really nice piece of software. I believe it is really useful to have all these CCA methods gathered at the same place.

I recommend acceptance.

As a side note, I believe the vision of mvlearn and cca-zoo coincide. It would be interesting to study how to fuse the two in my opinion.

A few comments:

robbisg commented 3 years ago

Hi everyone,

@emdupre I will not open an issue in the cca-zoo repo since I think that on my side there are few requests that can be addressed in this thread. But if you prefer, I can open the issue! ;)

Review

I think cca-zoo is a very useful package that can be used to relate multivariate datasets. Personally, I used very basic implementations of these technique and I am very happy for the possibility of using more advance technique included in this tool!

Paper

The paper is well written and presents an extensive comparison of other similar packages and the implementation philosophy. These edits should be included, in my point of view:

Software

The software uses the scikit-learn standard and this is very useful for the users, moreover CI and tests are valuable efforts to make the package more robust. I hadn't any test or import errors, and by inspecting github CI, it seems that everything is ok, also using different python versions.

I think that the tool is in a very good shape, I recommend acceptance.

Thanks @jameschapman19 and @emdupre, and sorry for the delay.

jameschapman19 commented 3 years ago

I was wary of replying as I don't know the process but it feels appropriate for me to say thanks both. All of these comments I agree with and will action them.

emdupre commented 3 years ago

Thanks both @hugorichard and @robbisg for your reviews, and @jameschapman19 for your thoughtful response !

Please do feel free to continue discussions in thread, though as specific issues on cca-zoo arise from this review process it will be helpful to cross-link them here !

emdupre commented 3 years ago

:wave: hi @ejolly ! I just wanted to check-in on this review.

Please let me know if you're encountering any technical difficulties that I can help with, or if you have a timeline for when you expect to be able to complete this !

ejolly commented 3 years ago

hey @emdupre sorry for the delay. I'm hoping to have this done by the end of this week or the start of next at the latest!

ejolly commented 2 years ago

Hey @emdupre @jameschapman19 here's my review:

Review

Suggested Changes

Minor suggested changes

Test and Issues

emdupre commented 2 years ago

Thanks @ejolly for your review !

It looks like https://github.com/jameschapman19/cca_zoo/issues/74, https://github.com/jameschapman19/cca_zoo/issues/75, https://github.com/jameschapman19/cca_zoo/issues/76, and https://github.com/jameschapman19/cca_zoo/issues/77 were all created in response to reviewer feedback, so I'll monitor those issues to see how the revisions are going.

@hugorichard @robbisg @ejolly if there are any additional issues you noted that you consider to not be covered by those issues, please let us know here !

jameschapman19 commented 2 years ago

@emdupre closed that final issue. Thanks for all of the suggestions I hope that they have all been addressed in some way.

emdupre commented 2 years ago

@whedon generate pdf

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:

emdupre commented 2 years ago

Thank you, @jameschapman19 !

@hugorichard and @robbisg I want to check with you explicitly since you have a few unchecked items on your reviewer checklist : do the provided revisions (as discussed in the cross-linked issues) address your previous concerns ? Of course, @ejolly, please also let us know if there's anything that you don't consider resolved !

ejolly commented 2 years ago

Looks good to me!

robbisg commented 2 years ago

Looks good to me too! :+1:

hugorichard commented 2 years ago

Looks good to me too!

emdupre commented 2 years ago

@whedon check references

whedon commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.2307/2333955 is OK
- 10.1016/0304-4076(76)90010-5 is OK
- 10.1162/0899766042321814 is OK
- 10.1007/s11336-011-9206-8 is OK
- 10.1093/biostatistics/kxp008 is OK
- 10.2202/1544-6115.1329 is OK
- 10.2202/1544-6115.1406 is OK
- 10.1111/biom.13043 is OK
- 10.3389/fninf.2016.00049 is OK
- 10.1007/978-1-4612-4228-4_3 is OK
- 10.1109/allerton.2015.7447071 is OK
- 10.1109/cvpr.2007.383137 is OK
- 10.1109/tnn.2007.891186 is OK

MISSING DOIs

- None

INVALID DOIs

- None
emdupre commented 2 years ago

Thank you @ejolly @hugorichard @robbisg for your thoughtful reviews and @jameschapman19 for your impressive work on cca-zoo !

At this point, @jameschapman19, could you please:

I can then move forward with accepting the submission :rocket:

jameschapman19 commented 2 years ago

Thanks all.

emdupre commented 2 years ago

@whedon set v1.10.7 as version

whedon commented 2 years ago

OK. v1.10.7 is the version.

emdupre commented 2 years ago

Thank you, @jameschapman19 !

Could you also please update the Zenodo archive title so it matches the title of the JOSS paper ? Specifically, "cca-zoo : a python package for implementing models from the canonical correlation analysis family".

jameschapman19 commented 2 years ago

Ah I see - done.

emdupre commented 2 years ago

@whedon set 10.5281/zenodo.5781712 as archive

whedon commented 2 years ago

OK. 10.5281/zenodo.5781712 is the archive.

emdupre commented 2 years ago

@whedon recommend-accept

whedon commented 2 years ago
Attempting dry run of processing paper acceptance...
whedon commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.2307/2333955 is OK
- 10.1016/0304-4076(76)90010-5 is OK
- 10.1162/0899766042321814 is OK
- 10.1007/s11336-011-9206-8 is OK
- 10.1093/biostatistics/kxp008 is OK
- 10.2202/1544-6115.1329 is OK
- 10.2202/1544-6115.1406 is OK
- 10.1111/biom.13043 is OK
- 10.3389/fninf.2016.00049 is OK
- 10.1007/978-1-4612-4228-4_3 is OK
- 10.1109/allerton.2015.7447071 is OK
- 10.1109/cvpr.2007.383137 is OK
- 10.1109/tnn.2007.891186 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 2 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/2827

If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/2827, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
kyleniemeyer commented 2 years ago

Hi @jameschapman19, I'm just doing some final checks before accepting.

Can you capitalize "python" in the paper's title? There are some uses throughout the paper where it should be capitalized as well.

Also, the Bach and Jordan reference needs a few more details. This is a technical report, published by University of California Berkeley Department of Statistics, Tech. Rep. 688. You can also add the URL https://statistics.berkeley.edu/sites/default/files/tech-reports/688.pdf.

Lastly, please confirm that there are no DOIs available for the Wenwen et al. and Wong et al. references.

emdupre commented 2 years ago

Thank you for catching those points, @kyleniemeyer !

Can you capitalize "python" in the paper's title? There are some uses throughout the paper where it should be capitalized as well.

@jameschapman19, when you're making those changes please update it in the zenodo archive as well, just so those two documents match !

jameschapman19 commented 2 years ago

Thanks @kyleniemeyer - I was able to track down a doi for Wenwen et al. too

@emdupre I made a related change to the references in the docstrings themselves so best to also change to 1.10.8 (but don't worry if it's too much trouble).

emdupre commented 2 years ago

@emdupre I made a related change to the references in the docstrings themselves so best to also change to 1.10.8 (but don't worry if it's too much trouble).

Thank you, @jameschapman19 ! To confirm, did you also create a corresponding archive for the new release ? Can you list the DOI here if so ?

jameschapman19 commented 2 years ago

New DOI: 10.5281/zenodo.5786616

Just realised there is also the DOI which resolves to the most recent: 10.5281/zenodo.4382739

I don't know what is more appropriate for JOSS but I'm happy with either.

kyleniemeyer commented 2 years ago

@whedon set 10.5281/zenodo.5786616 as archive

We prefer the DOI that points to the specific version

whedon commented 2 years ago

OK. 10.5281/zenodo.5786616 is the archive.

kyleniemeyer commented 2 years ago

@whedon generate pdf