search

openjournals / joss-reviews

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

[REVIEW]: Synaptus: A Matlab/Octave toolbox for synthetic aperture ultrasound imaging #4185

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@mh-skjelvareid<!--end-author-handle-- (Martin H. Skjelvareid) Repository: https://github.com/mh-skjelvareid/synaptus Branch with paper.md (empty if default branch): Version: v1.1 Editor: !--editor-->@Kevin-Mattheus-Moerman<!--end-editor-- Reviewers: @emanuelhuber, @Kevin-Mattheus-Moerman Archive: 10.5281/zenodo.6974195

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

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

@Kevin-Mattheus-Moerman & @emanuelhuber, 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 @Kevin-Mattheus-Moerman 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 @Kevin-Mattheus-Moerman

✨ 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 @emanuelhuber

✨ 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 2 years ago

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

Wordcount for paper.md is 1526

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

OK DOIs

- 10.1190/1.1440826 is OK
- 10.1190/1.1440899 is OK
- 10.1017/9781316756041 is OK
- 10.1109/TUFFC.2010.1718 is OK
- 10.1109/TUFFC.2007.400 is OK
- 10.1109/TUFFC.2011.1904 is OK
- 10.1109/TUFFC.2012.2478 is OK
- 10.1063/1.3703163 is OK
- 10.1016/j.ndteint.2012.10.005 is OK
- 10.1063/1.4928118 is OK
- 10.1109/ULTSYM.2017.8092389 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.06 s (617.3 files/s, 75709.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
MATLAB                          29            469           1133           1573
Markdown                         5            230              0            778
TeX                              1             18              0            191
YAML                             1              1              4             18
-------------------------------------------------------------------------------
SUM:                            36            718           1137           2560
-------------------------------------------------------------------------------

Statistical information for the repository '24166a9027d768e496096f49' was
gathered on 2022/02/22.
No commited files with the specified extensions were found.
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:

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner, @emanuelhuber this is where the review takes place. You can now start and you can post comments to the author here or open dedicated issues on their software repository and link to them here.

Please let me know if you have any questions. Thanks again for your help!!!!

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner, @emanuelhuber I hope you are doing well. Could you provide an update on how the review is going or if you have started yet? No worries if you haven't started yet, I am just checking in. Thanks again for your help.

emanuelhuber commented 2 years ago

@Kevin-Mattheus-Moerman I started the review, installed Octave . Hope to be finished by the end of the week. Best, Emanuel

emanuelhuber commented 2 years ago

(@Kevin-Mattheus-Moerman should I leave the check box "Installation instructions" unchecked if the requirement seems to be not completely fulfilled?

Here are some points I rise

Documentation

Software paper

Conclusion

The paper is well written, the code is documented, datasets are available, the examples work well, I could run everything although I am not an octave/matlab user and some effort has been done to explains some of the algorithms. I warmly thank the author for his contribution to the community!

mh-skjelvareid commented 2 years ago

@emanuelhuber , thank you for your valuable feedback! I will try to follow up on and correct each point that you have raised. Thanks also for being patient and flexible regarding installation of octave. As indicated in the paper, I'd like to (some day) update the toolbox to an open source (and perhaps more modern) programming language, for example Python. I haven't used Julia but I will definitely look into it.

@Kevin-Mattheus-Moerman : I guess I should wait for Samuel-Wagner to finish his review before I start changing anything?

emanuelhuber commented 2 years ago

@mh-skjelvareid , I startet programming with Matlab 20 years ago, then I switched to R (which is very similar to Matlab). Now I am learning Python for my job and I don't like it. I wrote a R-package (RGPR) and am still playing with the thought to translate it in another language. First I thought that Python would be great because many people in geosciences use it. But now I don't know... maybe write C/C++ functions that can be used in any language? Write a Julia package?

Kevin-Mattheus-Moerman commented 2 years ago

@emanuelhuber

... should I leave the check box "Installation instructions" unchecked if the requirement seems to be not completely fulfilled?

Yes, leave anything you are not happy with unchecked until resolved.

Kevin-Mattheus-Moerman commented 2 years ago

@mh-skjelvareid

I guess I should wait for Samuel-Wagner to finish his review before I start changing anything?

No you can start addressing @emanuelhuber 's comments right away if you like.

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner can you provide an update on how you are getting on with the review? It is okay if you need more time, just let us know, I am only checking in to see how things stand. Thanks again for your help.

mh-skjelvareid commented 2 years ago

@whedon generate pdf

editorialbot commented 2 years ago

My name is now @editorialbot

mh-skjelvareid commented 2 years ago

@editorialbot generate pdf

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:

mh-skjelvareid commented 2 years ago

@Kevin-Mattheus-Moerman , I have now tried to address the points raised by @emanuelhuber :

Documentation

  • readme.md file: In the summary the author explains that the toolbox is for a certain type of data but he does not explain what exactly the toolbox do (i.e., focusing these kind of data). I do not know if the toolbox can do more than focusing.

I have updateed the summary to clarify that the only purpose of the toolbox is to focus pulse-echo data.

  • There is no "clearly-stated list of dependencies": I had to install the 'signal' toolbox which required the 'control' toolbox. A short information about the dependence on signal and how to install and load the 'signal' toolbox would be welcome (why should I load manually the signal package?).

I have added "Requirements" section to the readme.md file which states that the toolbox requires Matlab / Octave and a signal toolbox to run. I've added a few practical details regarding installation of these.

  • I could not find any "external" API method documentation but the function are documented in their file and the three main focusing methods are described in the readme file and the examples allow to find the corresponding functions. It requires some extra "dig work" for novices to octave (like me). The simplified algorithms are surely a great ressource for the community.

I've added a "Documentation" section which states that the main toolbox functions are documented in the standard Matlab/Octave style (description at start of function). The documentation can be viewed on the Matlab/Octave command line by using the help command. I've also described how the "test" scripts are intended as a demonstration of how to use the toolbox.

  • Both file 'array_psm_testFullMatrix_1layer.m' and 'array_psm_testFullMatrix_2layer.m' have exactly the same note ('copper block') but load different files ('steel pins' and 'copper block'). Please check if the notes are correct.

The description in 'array_psm_testFullMatrix_1layer.m' was incorrect and has been updated - thanks!

  • When running 'cpsm_test2D.m' I got the following error : All image ranges processed Total processing time: 1.3711 seconds. error: text: Invalid combination of points and text strings error: called from text at line 128 column 7 polRawDataImage at line 93 column 1 cpsm_test2D at line 30 column 1

The polRawDataImage and polFocusedImage functions have been fixed to avoid this error.

Software paper Statement of need:

  • I suggest the author to repeat that synaptus is a toolbox for focusing...

I wasn't entirely sure what Emanuel meant by this comment, but I'm guessing he wanted a similar clarification as in the readme.md file (a statement that the toolbox is only used for creating focused images). I have tried to add this in the introduction to the Statement of need.

  • The author writes that Alain Plattner used "the code". This is misleading and it would be better to replace "the code" with the "Stolt fk migration function".

This passage has been updated as recommended.

I have added a paragraph to the paper referencing some of these toolboxes, and explaining that

emanuelhuber commented 2 years ago

@Kevin-Mattheus-Moerman Thank you very much for adressing all my comments. Everything is fine!

@mh-skjelvareid In my opinion, the paper is ready for publication.

Kevin-Mattheus-Moerman commented 2 years ago

@emanuelhuber thanks for your help. If you are happy to recommend publication, can you please tick all remaining boxes :point_up: ? Thanks again!

emanuelhuber commented 2 years ago

@Kevin-Mattheus-Moerman All check boxes are now checked!

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner how are you getting on? Are you able to start this review? Thanks!

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner how are you getting on? Are you able to start this review? Thanks!

Kevin-Mattheus-Moerman commented 2 years ago

I have just emailed @Samuel-Wagner a reminder about this review.

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner can you please update us on review progress?

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner can you please update us on review progress?

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner can you please update us on review progress?

:point_up:

Kevin-Mattheus-Moerman commented 2 years ago

@Samuel-Wagner Since we have not hear from you I will now assume you cannot review this work and will remove you as a reviewer.

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot remove @Samuel-Wagner as reviewer

editorialbot commented 2 years ago

@Samuel-Wagner removed from the reviewers list!

Kevin-Mattheus-Moerman commented 2 years ago

@arfon given that it has been challenging to find reviewers for this submission and that one of the reviewers was unable to complete the review I proposed that I will review this work as a second reviewer. Given my MATLAB and image analysis skills I feel I can review this quite well.

arfon commented 2 years ago

Sounds good to me @Kevin-Mattheus-Moerman 👍🏻

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot add @Kevin-Mattheus-Moerman as reviewer

editorialbot commented 2 years ago

@Kevin-Mattheus-Moerman added to the reviewers list!

Kevin-Mattheus-Moerman commented 2 years ago

@mh-skjelvareid I installed and tested the software and it all works really well after I implemented the below minor fix.

Main issue

%% Load test data load('../datasets/ArraySteelPins.mat','data','MWB')


with 
```matlab

%% Add path to necessary functions
toolboxPath=fileparts(fileparts(mfilename('fullpath'))); %Get the toolbox path

%Add core and misc path
addpath(fullfile(toolboxPath,'core'),fullfile(toolboxPath,'misc'));

%% Load test data
load(fullfile(toolboxPath,'datasets','ArraySteelPins.mat'),'data','MWB')

This fix should work for all operational systems.

Minor comments (not requirements):

mh-skjelvareid commented 2 years ago

@Kevin-Mattheus-Moerman Thanks a lot for the nice fix for the path definitions, and the other comments regarding consistency and testing. I've addressed the main point and some of the listed suggestions. I've checked the boxes for issues that have been fixed, and have added comments in between.

@mh-skjelvareid I installed and tested the software and it all works really well after I implemented the below minor fix.

Main issue

  • [x ] Can you merge this pull request, it fixes a bug for Ubuntu Linux. Namely that the path definitions did not work. I went through the various test codes and replaced stuff like:
%% Add path to necessary functions
addpath('../core','../misc')

%% Load test data
load('../datasets/ArraySteelPins.mat','data','MWB')

with

%% Add path to necessary functions
toolboxPath=fileparts(fileparts(mfilename('fullpath'))); %Get the toolbox path

%Add core and misc path
addpath(fullfile(toolboxPath,'core'),fullfile(toolboxPath,'misc'));

%% Load test data
load(fullfile(toolboxPath,'datasets','ArraySteelPins.mat'),'data','MWB')

This fix should work for all operational systems.

Done!

Minor comments (not requirements):

[x] Note that you currently use both caxis([minVal maxVal]) as well as set(gca,'CLim',[minVal maxVal]), for clarity you could consider just using one approach. The former (the use of axis) is labelled as deprecated for future MATLAB releases in favor of clim([minVal maxVal]). However, do check compatibility with Octave too which may still use the axis approach.

I've gone through the code and changed this to consistent use of caxis([minVal,maxVal]).

[x ] Note that clear all is the same as clear since all is default. Elsewhere you use clearvars which probably does the same thing again. But again, check Octave compatibility too.

I've gone through and changed the first lines of each script to "close all; clearvars". "clearvars" only clears variables while "clear" clears everything. It doesn't matter much for the test scripts, I think, but I've found clearvars to work better in some cases, so I've made it a habit.

[x ] You sometime have disp('Loading data'); and disp('Processing data'); as messages in the tests, but not always, consider using it for all tests.

I've added this for all the tests, and I've also standardized the measurement of processing time for each test (using only tic and toc).

[x ] You may consider, in future work, implementing automated testing, e.g. by running all tests automatically (see also something like: https://github.com/gibbonCode/GIBBON/blob/master/lib/testGibbon.m).

I've added a new script, "tests_runAll.m", which displays a "test suite header" and then runs each test one by one. Inside each of the tests I've also added a bit of code which displays a header. This gives a nice separation between the text output ("Processing layer 1 of 2" etc.) from each test.

  • [ ] You may also consider, in future work, making the tests "publishable" in the MATLAB code publishing sense, this way they can turn in to HTML files that you can render as documentation too. Once again GIBBON might be an example.

This seems like a nice solution. I'm not sure if I understand exactly how it works, though. Is all the documentation generated through (Matlab) publishing during installation? If so I guess it won't work for Octave? I get the sense that this could make for some nice documentation, but that it'll be a bit too much work for me to implement at this stage (the toolbox is no longer part of my active research work).

  • [ ] Similarly you may consider automated script based installation (which could simple be adding the toolbox paths and saving them).

I agree that this would make the installation a bit smoother. However, I have no experience in making installation scripts, and I'm expecting that it's hard to make something that works in absolutely all cases...(?) I think I'd prefer for the users to add the toolbox to the path of their particular installation of Matlab / Octave themselves.

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot generate pdf

Kevin-Mattheus-Moerman commented 2 years ago

@mh-skjelvareid thanks for implementing those changes and for your comments. I tested the changes this time using Octave on Ubuntu, rather than MATLAB, and all tests passed fine and the changes look good.

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:

Kevin-Mattheus-Moerman commented 2 years ago

@mh-skjelvareid I have proofread the paper and here are some last points and recommendations:

mh-skjelvareid commented 2 years ago

@Kevin-Mattheus-Moerman, thanks for finding all these mistakes, I've corrected them all, and have also corrected a couple of other mistakes I found during spellchecking. I went for consistency with American English.

@mh-skjelvareid I have proofread the paper and here are some last points and recommendations:

  • [x] Please complete the affiliation with the full name of the institute, the city name, and the country.
  • [x] Remove the statement: NOTE: This is a draft version of a paper submitted to the Journal of Open Source Software (JOSS).
  • [x] Check more computationally efficient that operating which should read more computationally efficient than operating
  • [x] Check resouce -> resource
  • [x] Check cartesian -> `Cartesian'
  • [x] Check procesing -> processing
  • [x] Check accomodate -> accommodate
  • [x] Can you check for consistency with either American or European (Brittish/Irish) English? E.g. You use optimized, vectorized, and specialized which favors American English, whereas the use of Aluminium over Aluminum favours Brittish English. Either is fine but it is best to be consistent.
  • [x] Check Overview of experiment setup, this should perhaps be improved to something like Overview of the experimental setup
  • [x] Check Seen from side -> Seen from the side
  • [x] Check ultrasonic raw data -> raw ultrasonic data
mh-skjelvareid commented 2 years ago

@editorialbot generate pdf

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:

Kevin-Mattheus-Moerman commented 2 years ago

@mh-skjelvareid it looks good except Overview of the experiment setup should still be changed to Overview of the experimental setup.

mh-skjelvareid commented 2 years ago

@Kevin-Mattheus-Moerman Sorry about that, fixed now.

mh-skjelvareid commented 2 years ago

@editorialbot generate pdf

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:

Kevin-Mattheus-Moerman commented 2 years ago

@mh-skjelvareid Now that all reviewers are happy with both the software and the paper we can proceed to process this for acceptance in JOSS. Here are some action items for you:

Thanks.

mh-skjelvareid commented 2 years ago

@Kevin-Mattheus-Moerman , see replies below:

@mh-skjelvareid Now that all reviewers are happy with both the software and the paper we can proceed to process this for acceptance in JOSS. Here are some action items for you:

  • [x] Please archive a copy of the software on ZENODO. You can do this manually if you like but some find these automated steps useful.
  • [x] Please check and edit the ZENODO archive so that the title and the authors match those of the paper. Also check that the license is correct. Furthermore we recommend that you add your ORCID information to the ZENODO archive too.
  • [x] Once you have the archived version created, please report back here with the archive DOI.

All done. The DOI is 10.5281/zenodo.6974194 I've added the DOI badge at the top of the ´README.md´ file: DOI

I think I got all the details right, but please check that I did. My chosen license is GNU GPL v3, but this wasn't available in the drop-down menu of possible licenses, so I chose the closest one, "GNU General Public License v2.0 or later"

  • [x] The current version tag is v1.1.0, is this still accurate or should we update this? Note the automated approach above may cause you to issue an updated version so please inform us what the version tag should be.

I believe that until now, the version has been v1.0 ("Initial release"), but I've created another release in order to get the Zenodo DOI, so the current (latest) version is v1.1. Does it look OK?