openjournals / joss-reviews

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

[REVIEW]: Efficiently Collecting Relative Similarity Judgments from Humans #4517

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@stsievert<!--end-author-handle-- (Scott Sievert) Repository: https://github.com/stsievert/salmon/ Branch with paper.md (empty if default branch): master Version: v1.0.2 Editor: !--editor-->@ajstewartlang<!--end-editor-- Reviewers: @hoechenberger, @stain, @jorgedch Archive: 10.5281/zenodo.7832431

Status

status

Status badge code:

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

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

@hoechenberger,@jorgedch, & @stain, 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 @ajstewartlang 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 @hoechenberger

📝 Checklist for @jorgedch

📝 Checklist for @stain

editorialbot commented 2 years 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 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.15 s (775.8 files/s, 118657.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          50           1856           2614           6491
HTML                             5             93             12           1246
reStructuredText                15            547            600            811
CSS                              2            178              0            783
YAML                            26             35             49            554
Jupyter Notebook                 3              0           1391            245
TeX                              1             20              0            181
Markdown                         6             58              0            165
Bourne Shell                     3             20             48             73
make                             2             14             10             39
DOS Batch                        1              8              1             26
Dockerfile                       2              6              2             22
SVG                              3              0              0              3
-------------------------------------------------------------------------------
SUM:                           119           2835           4727          10639
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 887

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

OK DOIs

- 10.1137/1.9781611974010.31 is OK
- 10.1109/MLSP.2012.6349720 is OK
- 10.1007/s42761-020-00024-8 is OK
- 10.1109/BTAS.2016.7791205 is OK
- 10.1109/IJCNN.2019.8852059 is OK
- 10.1111/cogs.12744 is OK
- 10.25080/shinma-7f4c6e7-010 is OK
- 10.1109/TKDE.2019.2956700 is OK
- 10.1109/Allerton.2011.6120287 is OK

MISSING DOIs

- None

INVALID DOIs

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

ajstewartlang commented 2 years ago

@editorialbot add @jorgedch as reviewer

editorialbot commented 2 years ago

@jorgedch added to the reviewers list!

jorgedch commented 2 years ago

Review checklist for @jorgedch

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

jorgedch commented 2 years ago

@stsievert

Functionality

  1. https://github.com/stsievert/salmon/issues/135
  2. I'll go through the Functionality and Performance checklist items once the Installation is fixed 👍

Documentation

  1. https://github.com/stsievert/salmon/issues/136

Software paper

  1. https://github.com/stsievert/salmon/issues/137
  1. https://github.com/stsievert/salmon/issues/138
stsievert commented 2 years ago

Salmon's documentation mentions an install process on Amazon AWS: https://docs.stsievert.com/salmon/installation.html#experimentalist. I've reached out to the reviewers (@hoechenberger, @stain, @jorgedch) to provide an Amazon AWS account so they can more easily verify that process works and because Amazon EC2 machines have a non-zero cost.

@ajstewartlang let me know if that violates any ethics rules; I can revoke their permissions at any time. The relevant AMI is public (see Amazon AWS Dashboard screenshot below), so the fact I created their accounts should not provide privileged access not available to the public.

Screen Shot 2022-07-07 at 10 49 08 PM
ajstewartlang commented 2 years ago

Thanks @stsievert - for the purpose of facilitating the review process, I'm comfortable with that. I'll assume that @hoechenberger, @stain, and @jorgedch are too unless I hear otherwise.

jorgedch commented 2 years ago

@stsievert the solutions to the following issues are already posted in their corresponding threads:

Functionality: https://github.com/stsievert/salmon/issues/140 https://github.com/stsievert/salmon/issues/141

Feel free to copy/paste the code in the relevant files and close the issues. Other than that, everything else is approved from my side and the code is ready for publication. All the best, Jorge

ajstewartlang commented 2 years ago

:wave:

@stain and @hoechenberger - just checking in to see how your reviews of this submission are going?

hoechenberger commented 2 years ago

@ajstewartlang I'm sorry for the delay, I'm working on the review right now and will submit ASAP!

hoechenberger commented 2 years ago

Review checklist for @hoechenberger

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

hoechenberger commented 2 years ago

Hello @stsievert @ajstewartlang,

I am happy with everything except with the installation procedure. I managed to get Salmon to run on an AWS instance, but local installation failed, both via Docker and via conda.

There is a problem when running docker-compose build. I'm pasting the relevant parts here:

#0 219.1 Installing pip dependencies: ...working... Pip subprocess error:
#0 317.7 ERROR: Cannot uninstall 'PyYAML'. It is a distutils installed project and thus we cannot accurately determine which files belong to it which would lead to only a partial uninstall.
...
#0 317.7   Attempting uninstall: pyyaml
#0 317.7     Found existing installation: PyYAML 5.1.2
#0 317.7
#0 317.7 failed
#0 317.7
#0 317.7 CondaEnvException: Pip failed
#0 317.7
------
failed to solve: executor failed running [/bin/sh -c conda env update -n base --file /salmon/salmon.yml --prune]: exit code: 1

And for installation from the salmon.yaml file via conda, I run into:

Encountered problems while solving:
  - nothing provides requested bokeh 2.0.1

This is on macOS 12.6 on an Apple Silicon machine.

Since the AWS installation worked flawlessly, I was still able to review functionality of the software.

stsievert commented 2 years ago

@hoechenberger thanks for the review! I'm glad to hear (almost) everything went smoothly. I've fixed the conda install issue I think, and released v1.0.0rc5. What version of conda do you have?

ajstewartlang commented 2 years ago

:wave: @stain how is your review going?

stain commented 2 years ago

Review checklist for @stain

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

stain commented 2 years ago

Sorry for my late reviews - github emails got drowned in the wrong mailbox

Although I am not an expert on this domain I have reviewed this mainly from a Web developer aspect. The author has taken many steps to make this software usable for third-party users and I am quite happy to recommend this paper.

A few small improvements can still be done as detailed below, but I would not put them as blocker for the paper.

The only concern I have is for the documentation URL, it is self-hosted but currently excluded from Web archives.

Usability for surveyed users

I was surprised how the endpoint for users is thrown straight into the question -- perhaps the documentation or paper can suggest to what extent these screens can/should be customized, e.g. to explain the particular crowd sourcing experiment, to add privacy statements etc.

AWS install

I have not tested the AWS install as I prefer self-hosting.

Pip or Conda or Docker?

It's good with many install options, but it can get confusing.

https://docs.stsievert.com/salmon/installation is different from https://docs.stsievert.com/salmon/offline.html which recommends first pip, but this fails in one of the many dependencies, bokeh, similar to @hoechenberger above:

Building wheels for collected packages: bokeh, blosc, python-multipart, tornado
  Building wheel for bokeh (setup.py) ... done
  Created wheel for bokeh: filename=bokeh-2.0.1-py3-none-any.whl size=9080019 sha256=1919f56df60581979e945e10b53524d9bcf4b2e2305aff2a28bd9d74eb14f54e
  Stored in directory: /home/stain/.cache/pip/wheels/be/b4/d8/7ce778fd6e637bea03a561223a77ba6649aff8168e3c613754
  Building wheel for blosc (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Building wheel for blosc (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [84 lines of output]
      Not searching for unused variables given on the command line.
      -- The C compiler identification is unknown
      CMake Error at CMakeLists.txt:3 (ENABLE_LANGUAGE):
        No CMAKE_C_COMPILER could be found.

This is on Ubuntu 20.04.5 TLS on WSL2 with Python 3.10.4. I did not proceed e.g. with installing cmake as I tend to move to Conda if I need many binary dependencies.

A warning should be added to the documentation in offline.html that there are many Python dependencies which are not always trivial to install, and that the Conda approach should be tried first.

It may be worth later to tidy up the list of dependencies, are they all actually needed? A big list can make it hard to use salmon with other software as there can be version mismatches.

You should declare the license in setup.py so it shows up in metadata at https://pypi.org/project/salmon-triplets/ e.g. adding classifier License :: OSI Approved :: MIT License

Likewise the license is not declared on the documentation page, only on GitHub. On the documentation it is worth pointing out that dependencies will have other open source licenses.

I did not check the license of all the dependencies are compatible, but it is worth checking:

as using this library could make all your source code also GP3 rather than MIT.

Docker install

Following the main guide https://docs.stsievert.com/salmon/installation#local-machine I did a Docker install. As this is effectively a containerized Conda install, all the benefits and challenges of that apply, see below.

Docker Compose build and up took slightly longer than Conda install, but completed well.

Conda install

Conda install worked without any trouble.

The salmon.yml has reasonable version locking where needed. However, as the Conda packages available may change over time, it is also useful to record what exact versions worked by the author for all dependencies.

Try adding a salmon.yml.lock file by doing:

conda env export > salmon.yml.lock

Paper

The paper is well written, but could reflect better if not just AWS install is possible, or what are the limitation of local/offline usage.

Documentation pages

The documentation pages https://docs.stsievert.com/salmon/ are well structured and easy to follow.

The documentation links from the Paper are hosted at stsievert.com which is the author's personal home page. While I encourage the practice of self-hosting, this domain seems to not be included in the Wayback machine and therefore a reasonable question will be about longevity of these documentation links. (I've tried adding it)

I would recommend checking why the domain is exluded from Wayback Machine archiving (e.g. robots.txt which disallows all access). I would also link to the documentation using https://w3id.org/ permalinks in case the domain name is lost.

stsievert commented 2 years ago

Thanks for the feedback @stain! Especially for the feedback on the paper, conda install lock file, and experiment manager documentation. I've added two documentation pages, and have some more notes in https://github.com/stsievert/salmon/pull/139.

Two more notes:

arfon commented 1 year ago

:wave: @stsievert - this submission looks pretty close to being ready to accept. How are you getting on with your last set of changes?

stsievert commented 1 year ago

@editorialbot set review2 as branch

editorialbot commented 1 year ago

Done! branch is now review2

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

stsievert commented 1 year ago

Thanks for the ping @arfon! I've resolved of the concerns mentioned above (and also some concerns a Salmon user had). I've made some edits to the paper, and they're available above.

One important note: I've changed from an MIT license to an Apache BSD license.

"therefore a reasonable question will be about longevity of these documentation links."—@stain

I've added a PDF rending of the documentation to GitHub, and link to it from the docs main page so people are aware.

"I did not check the license of all the dependencies are compatible, but it is worth checking:"—@stain

I did check them all, they're available at https://docs.stsievert.com/salmon/deps. And I've added that to the documentation home page too: https://docs.stsievert.com/salmon/index.html

"The salmon.yml has reasonable version locking where needed. However, as the Conda packages available may change over time,"—@stain

Good idea, implemented. Thank you. I've also minimized the dependencies (and tried to make the install more clear too).

@hoechenberger I've resolved a bunch of issues with the local installation you mentioned in https://github.com/openjournals/joss-reviews/issues/4517#issuecomment-1246725742. Could you give it a try again?

More complete notes are at the top of https://github.com/stsievert/salmon/pull/139 (and a new PR at https://github.com/stsievert/salmon/pull/142)

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

stsievert commented 1 year ago

I've added Profs. Robert Nowak and Timothy Rogers as authors after more closely reviewing the authorship guidelines; while they are not mentioned in the commit history; however they actively contributed to the project direction.

ajstewartlang commented 1 year ago

:wave: @hoechenberger could you try installing locally again please? I believe that's the last thing we need to tick off on this submission.

hoechenberger commented 1 year ago

Hello @ajstewartlang, it's all working for me now! Sorry for the late response, it seems I dismissed the notification and then it got lost.

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

ajstewartlang commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1137/1.9781611974010.31 is OK
- 10.1109/MLSP.2012.6349720 is OK
- 10.1007/s42761-020-00024-8 is OK
- 10.1109/BTAS.2016.7791205 is OK
- 10.1109/IJCNN.2019.8852059 is OK
- 10.1111/cogs.12744 is OK
- 10.25080/shinma-7f4c6e7-010 is OK
- 10.1109/TKDE.2019.2956700 is OK
- 10.1109/Allerton.2011.6120287 is OK

MISSING DOIs

- None

INVALID DOIs

- None
ajstewartlang commented 1 year ago

Many thanks for your very helpful reviews @stain, @jorgedch, and @hoechenberger. @stsievert many thanks for your great submission - I think we're almost good to go...

ajstewartlang commented 1 year ago

:wave: @stsievert

If you could now do the following please, that would be great:

stsievert commented 1 year ago

Thanks @ajstewartlang! I've gone through some trouble, and released Salmon v1.0.2 with the DOI 10.5281/zenodo.7832320 (which points to the latest version; the v1.0.2 DOI is 10.5281/zenodo.7832431).

I have checked the metadata and changed the authors (related the reason I had to do more than one release; now Github integration is turned off!).

ajstewartlang commented 1 year ago

@editorialbot set v1.0.2 as version

editorialbot commented 1 year ago

Done! version is now v1.0.2

ajstewartlang commented 1 year ago

@editorialbot set 10.5281/zenodo.7832431 as archive

editorialbot commented 1 year ago

Done! Archive is now 10.5281/zenodo.7832431

ajstewartlang commented 1 year ago

@editorialbot recommend-accept

editorialbot commented 1 year ago
Attempting dry run of processing paper acceptance...
editorialbot commented 1 year ago

Couldn't check the bibtex because branch name is incorrect: review2

editorialbot commented 1 year ago

:warning: Error preparing paper acceptance.

ajstewartlang commented 1 year ago

@stsievert can you confirm which branch the final version of the paper (i.e., the final version of paper.md) is in please? The branch review2 appears to be empty. I think it's now in master but please let me know for sure. Thanks!

stsievert commented 1 year ago

Whoops! Good catch. Correct, review2 has been merged into master in https://github.com/stsievert/salmon/pull/142

stsievert commented 1 year ago

@editorialbot set master as branch