Joss-reviews: [REVIEW]: Tools for Prototyping with 3D Ultrasonics in ROS

Created on 27 Jun 2019  ยท  109Comments  ยท  Source: openjournals/joss-reviews

Submitting author: @adi3 (Adi Singh)
Repository: https://gitlab.com/toposens/public/ros-packages
Version: v1.2.1
Editor: @gkthiruvathukal
Reviewer: @dvberkel
Archive: 10.5281/zenodo.3336380

Status

status

Status badge code:

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

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

@dvberkel, 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 @gkthiruvathukal know.

โœจ Please try and complete your review in the next two weeks โœจ

Review checklist for @dvberkel

Conflict of interest

Code of Conduct

General checks

  • [x] Repository: Is the source code for this software available at the repository url?
  • [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [x] Version: v1.2.1
  • [x] Authorship: Has the submitting author (@adi3) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation?
  • [x] Functionality: Have the functional claims of the software been confirmed?
  • [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • [x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • [x] Authors: Does the paper.md file include a list of authors with their affiliations?
  • [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
accepted published recommend-accept review

All 109 comments

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @dvberkel it looks like you're currently assigned to review this paper :tada:.

: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
Attempting PDF compilation. Reticulating splines etc...

PDF failed to compile for issue #1531 with the following error:

Can't find any papers to compile :-(

@whedon generate pdf from branch joss_paper

Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...

@gkthiruvathukal @dvberkel Thanks for getting the review started :)
Shouldn't it mention @nnadeau as a reviewer too? Or are the reviews done sequentially?

@ts-adi I am looking for community guidelines in your repository. But I can't find them. Am I missing something, or is a CONTRIBUTING.md file missing?

@gkthiruvathukal I don't have a Toposens TS3 sensor, so how can I can I review if the functionality claims of the software?

@ts-adi Is there a possibility to send a sensor for me to review?

@dvberkel Just merged master changes to the joss_paper branch. We've taken away most of the documentation from the readme and put it on the ROS wiki (and provided a link to it from the git readme). See here:

This avoids repetition of the same content in two places. Our wiki page mentions (and links to) the repo's bug/feature tracker. I haven't explicitly added a CONTRIBUTING.md file because instructions to contribute our laid by the ROS wiki already here: http://wiki.ros.org/Contributing

Do you suppose adding a link to this in our readme should suffice? I would much rather add content to the readme rather than add a separate file.

@gkthiruvathukal What is the procedure for testing open source software built for commercial hardware devices?

We do have an extensive automated test suite in the repo that emulates the sensor (straightforward posix emulation) for our continuous integration needs. These tests run on a docker container and as such have no direct access to the hardware. Through emulation, we ensure tests can run hardware-free and still confirm that our functionality is not broken. Do you suppose going through the test suite might assist in confirming our functionality claims?

@dvberkel Let's see what suggestions come from the editor. We'll find a way forward accordingly :)

@dvberkel Just merged master changes to the joss_paper branch. We've taken away most of the documentation from the readme and put it on the ROS wiki (and provided a link to it from the git readme). See here:

* https://gitlab.com/toposens/public/ros-packages/tree/joss_paper

* http://wiki.ros.org/toposens

This avoids repetition of the same content in two places. Our wiki page mentions (and links to) the repo's bug/feature tracker. I haven't explicitly added a CONTRIBUTING.md file because instructions to contribute our laid by the ROS wiki already here: http://wiki.ros.org/Contributing

I am all in favor of not duplicating information :+1:

Do you suppose adding a link to this in our readme should suffice? I would much rather add content to the readme rather than add a separate file.

For me a link would more than suffice.

@ts-adi and @dvberkel, To answer this question fully, I would need to know whether the commercial devices are available to the public or are still in the lab. If there is an emulation layer (that is, either a library or a hardware emulator) that can be used to run the tests, I think this would be satisfactory.

Ultimately, what I would be checking is whether there is a strong emphasis on testing, which in many cases can also be done by inspection. So if I am understanding it right: (a) we have proper unit tests / system tests, and (b) we can run the tests using an emulation layer within a Docker container? If both of these apply--as I think they do--this more than meets our editorial standards.

Lastly, is there some sort of continuous integration process that you use, @ts-adi, where you might be able to show the results of building and testing the software on your own hardware? This would give us an additional level of confidence.

Thanks for checking with me! Software engineering is my thing, and I always enjoy hearing about interesting cases like this one, which I think will be of interest to JOSS readers in general.

@gkthiruvathukal there is an extensive continuous integration process that ticks both your requirements.

I will dive into the looking into the tests more thoroughly

@gkthiruvathukal thank you for the detailed guidance. Here are specific answers to your questions:

  • The device is available to the public: http://toposens.com/ts3

  • We have tests with a wide code coverage and they run within a docker container using the emulation. Apart from our own CI setup in gitlab, these tests are automatically triggered when an update is merged in a ROS distro. See results of our 42 tests here. If you click on the green label saying "Continuous Integration", you can also view individual Jenkins jobs on the ROS buildfarm.

As for as continuous testing of the software on real hardware is concerned, this is how we do it:

  • Anytime there is a firmware update on the device, we record data dumps coming from the sensor and upload it to a remote server: http://85.214.248.206/ros/

  • Here is an example of this recording where we inform this Makefile that a device is plugged in:

````cmake
if(PLUGGED_IN)
set(LOG_PERIOD 3s)
set(PORT /dev/ttyUSB0)

message(STATUS "Extracting data from live stream for ${LOG_PERIOD}")
execute_process(
      COMMAND timeout ${LOG_PERIOD} roslaunch ${PROJECT_NAME} ${PROJECT_NAME}.launch
  COMMAND timeout ${LOG_PERIOD} cat ${PORT}
  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
  OUTPUT_FILE raw_stream
)
message(STATUS "Raw stream extraction complete")

endif()
````

  • When tests are run, the Makefile downloads this test data from the server and runs all our integration and some relevant unit tests using these files:
catkin_download_test_data(
  ${PROJECT_NAME}_raw_stream
  ${TEST_DATA_LOCATION}/raw_stream
  DESTINATION ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/tests
    MD5 3ec2f56b1cf5372cc98499c4458d37ff
)

catkin_download_test_data(
  ${PROJECT_NAME}_ts_scans.bag
  ${TEST_DATA_LOCATION}/ts_scans.bag
  DESTINATION ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/tests
    MD5 1fa17f252671369fa754e5e0736dbb7c
)

This process keep us confident that hardware and firmware updates ultimately do not break our codebase since rosbag files hold the entire state of the ROS system (all comms, all frequencies, all device info) for the duration of a recording.

Hope this gives a little more clarity on the project and helps you wade through our tests @dvberkel

@gkthiruvathukal By the way, wanted to follow up on my earlier question about the second reviewer. Should he be assigned to this review as well?

@dvberkel I have also added a few lines at the end of the readme to satisfy the community guidelines condition as per our discussion above: https://gitlab.com/toposens/public/ros-packages/tree/joss_paper

@ts-adi I went with one reviewer on this submission because I understand the subject matter well and can fill in missing details (as I was able to do on the testing matter). I feel like we're moving in the direction of acceptance and are getting great feedback from our reviewer, so are you on with continuing with just one, union input from me?

@dvberkel I have also added a few lines at the end of the readme to satisfy the _community guidelines_ condition as per our discussion above: https://gitlab.com/toposens/public/ros-packages/tree/joss_paper

This is excellent!

@ts-adi I went with one reviewer on this submission because I understand the subject matter well and can fill in missing details (as I was able to do on the testing matter). I feel like we're moving in the direction of acceptance and are getting great feedback from our reviewer, so are you on with continuing with just one, union input from me?

@gkthiruvathukal Yes, this sounds reasonable to me. Thanks!

@ts-adi I am not very familiar with Robotic Operating System. I have read some of the tutorial material. My understanding of what your project entails is that it offers a standard interface to read sensor data.

Am i correct in my assumption that the for the * Functionality documentation* part of the review. I should review that it adheres to the ROS framework guidelines? I.e. there is no documentation other than expect to fit in with other ROS sensors and data?

@dvberkel Your understanding is correct. It standardizes the following things:

  • reading and parsing incoming data from a TS3 sensor
  • fine tuning wave or signal processing parameters directly from ROS
  • demonstrates integration with popular ROS tools like rviz, pcl2, tf2, etc

No other documentation to go through except what we have in the ROS wiki (http://wiki.ros.org/toposens) and the general ROS framework guidelines. However, what I understand from the editor's previous comments is that you should simply ensure that functionality of the libraries is adequately tested by the automated testing suite.

@dvberkel Your understanding is correct. It standardizes the following things:

* reading and parsing incoming data from a TS3 sensor

* fine tuning wave or signal processing parameters directly from ROS

* demonstrates integration with popular ROS tools like rviz, pcl2, tf2, etc

No other documentation to go through except what we have in the ROS wiki (http://wiki.ros.org/toposens) and the general ROS framework guidelines.

Great :+1:

However, what I understand from the editor's previous comments is that you should simply ensure that functionality of the libraries is adequately tested by the automated testing suite.

This is true, but there is also a review point on the documentation side.

@dvberkel How was your experience with the software? Can I help you anywhere with the installation or running a functionality? I know ROS has a pretty steep learning curve.

@gkthiruvathukal @dvberkel Can we get an update on this when you have a moment please?

@ts-adi I am satisfied with your responses to my earlier questions about testing. I'm confident that there is a good process behind this work (from a software engineering standpoint).

I'm looking through the comments to see whether @dvberkel's issues are all addressed, so we can move toward acceptance. Upon hearing from @dvberkel, the next step will be to have you create a Zenodo archive for the submission.

Sorry for the late reply. I was moving and was unexpectedly longer than anticipated without internet. I have not reservation about the functionality.

The only thing that I haven't checked is the references. But I think whedon can check those automatically? @gkthiruvathukal Is this correct.

@dvberkel Thanks for getting back on this (and hope your move went smooth). I'll try to run the whedon command and see what it produces.

@whedon check references

Attempting to check references...

Doesn't look like it worked. @gkthiruvathukal any pointers?

Doesn't look like it worked. @gkthiruvathukal any pointers?

I don't think @whedon knows how to check references when the paper isn't on the default (master) branch sorry.

@arfon How should the reviewers check for the references then? Would it make sense if I temporarily commit the paper (and .bib) to the master branch?

A separate question for @arfon and @gkthiruvathukal This paper review was started when the software was at version 1.1.0. Since then, we have added a few minor things and fixed bugs and the repo is currently at version 1.2.1 (no effect on paper content though). So when I index the software on Zenodo, would it be for v1.1.0 or v1.2.1?

@whedon check references from branch joss_paper

Attempting to check references...

@whedon check references from branch joss_paper

Attempting to check references...

@whedon check references from branch joss_paper

Attempting to check references... from custom branch joss_paper

```Reference check summary:

OK DOIs

  • 10.1007/s11235-015-0034-5 is OK
  • 10.1109/ICRA.2011.5980567 is OK
  • 10.1109/TePRA.2013.6556373 is OK
  • 10.1109/MSPEC.2017.8000281 is OK

MISSING DOIs

  • None

INVALID DOIs

  • None
    ```

@arfon How should the reviewers check for the references then? Would it make sense if I temporarily commit the paper (and .bib) to the master branch?

I've added this to @whedon's capabilities now :point_up:

Since then, we have added a few minor things and fixed bugs and the repo is currently at version 1.2.1 (no effect on paper content though). So when I index the software on Zenodo, would it be for v1.1.0 or v1.2.1?

I think it's fine to archive v1.2.1

```Reference check summary:

OK DOIs

  • 10.1007/s11235-015-0034-5 is OK
  • 10.1109/ICRA.2011.5980567 is OK
  • 10.1109/TePRA.2013.6556373 is OK
  • 10.1109/MSPEC.2017.8000281 is OK

MISSING DOIs

  • None

INVALID DOIs

  • None
    ```

```Reference check summary:

OK DOIs

  • 10.1007/s11235-015-0034-5 is OK
  • 10.1109/ICRA.2011.5980567 is OK
  • 10.1109/TePRA.2013.6556373 is OK
  • 10.1109/MSPEC.2017.8000281 is OK

MISSING DOIs

  • None

INVALID DOIs

  • None
    ```

@arfon Great work extending whedon's functionality so quickly!
@gkthiruvathukal @dvberkel Looks like references all check out

@arfon @gkthiruvathukal Currently in the process of archiving the submission on Zenodo. Should I index the software release from the master branch? Or should this be from the custom joss_paper branch that contains the contents of master + the JOSS paper?

@ts-adi, If the plan is to continue development and tagging releases from master, I would use master. You don't need to keep the JOSS paper in your archive. Thanks!

@gkthiruvathukal The submission is now archived: https://zenodo.org/record/3336380#.XS3VwvyxVhE

JOSS requires the DOI, not the URL...

Ok, here you go: 10.5281/zenodo.3336380

I am giving the all green from my side :+1:

@whedon commands

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to accept the paper and deposit with Crossref
@whedon accept

# Ask Whedon to check the references for missing DOIs
@whedon check references

@whedon set 10.5281/zenodo.3336380 as archive

OK. 10.5281/zenodo.3336380 is the archive.

Appreciate all your help with this @dvberkel !

@gkthiruvathukal thanks for setting the archive. What happens next?

@ts-adi I will follow up shortly. We have a few final checks, which I will post when I get back to work tomorrow. We should have this wrapped up soon.

Ok, so here is the final checklist. Can you please do the following? (This is something I am doing for all submissions now, so you can just check the ones you already did above.)

  • [ ] Make a tagged release of your software, and list the version tag of the archived version here.
  • [ ] Archive the reviewed software in Zenodo
  • [ ] Check the Zenodo deposit has the correct metadata, this includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it); you may also add the authors' ORCID.
  • [ ] List the Zenodo DOI of the archived version here.

Once these are done, I will do a final proofread and sign off on the submission.

@whedon set v1.2.1 as version

I'm sorry @ts-adi, I'm afraid I can't do that. That's something only editors are allowed to do.

@gkthiruvathukal Here's the checklist. The only remaining step as I see it is setting the version here to 1.2.1 to match the zenodo archive. Looks like only you can instruct @whedon to do so.

  • [x] Make a tagged release of your software, and list the version tag of the archived version here.

  • [x] Archive the reviewed software in Zenodo

  • [x] Check the Zenodo deposit has the correct metadata, this includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it); you may also add the authors' ORCID.

  • [x] List the Zenodo DOI of the archived version here.

@whedon set v1.2.1 as version

OK. v1.2.1 is the version.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

PDF failed to compile for issue #1531 with the following error:

Can't find any papers to compile :-(

@whedon generate pdf from branch joss_paper

Attempting PDF compilation from custom branch joss_paper
. Reticulating splines etc...

PDF failed to compile for issue #1531 with the following error:

error: pathspec 'joss_paper
' did not match any file(s) known to git.
Can't find any papers to compile :-(

@ts-adi Are you able to generate the paper PDF? I cannot seem to do so now. I'm going to see if the @openjournals/joss-eics can help us figure this out.

@whedon generate pdf from branch joss_paper

Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...

@ts-adi Are you able to generate the paper PDF? I cannot seem to do so now. I'm going to see if the @openjournals/joss-eics can help us figure this out.

@gkthiruvathukal looks like there was an extraneous /n character in the command you issued. I've generated the pdf above. Feel free to give it a shot yourself too.

@whedon generate pdf from branch joss_paper

Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...

@ts-adi No idea how that extraneous newline got in there. All appears to be in working order now. I've checked the proof and everything appears to be fine from my checklist.

@openjournals/joss-eics I recommend this submission for acceptance.

Hi @adi3 @ts-adi I just submitted a minor PR that made some edits to the bib file, could you merge that? https://gitlab.com/toposens/public/ros-packages/merge_requests/83

@kyleniemeyer done!

@whedon generate pdf from branch joss_paper

@whedon generate pdf from branch joss_paper

Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...

@kyleniemeyer Also merged now. Appreciate your due diligence with this.

@whedon generate pdf

Attempting PDF compilation. Reticulating splines etc...

PDF failed to compile for issue #1531 with the following error:

Can't find any papers to compile :-(

@whedon generate pdf from branch joss_paper

@whedon generate pdf from branch joss_paper

Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...
Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...

OK, all looks good, so I'll proceed with accepting.

@whedon accept

Attempting dry run of processing paper acceptance...

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

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

@whedon accept deposit=true

Doing it live! Attempting automated processing of paper acceptance...

๐Ÿฆ๐Ÿฆ๐Ÿฆ ๐Ÿ‘‰ Tweet for this paper ๐Ÿ‘ˆ ๐Ÿฆ๐Ÿฆ๐Ÿฆ

๐Ÿšจ๐Ÿšจ๐Ÿšจ THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! ๐Ÿšจ๐Ÿšจ๐Ÿšจ

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited :point_right: https://github.com/openjournals/joss-papers/pull/855
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01531
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! ๐ŸŽ‰๐ŸŒˆ๐Ÿฆ„๐Ÿ’ƒ๐Ÿ‘ป๐Ÿค˜

    Any issues? notify your editorial technical team...

@arfon is there an issue with whedon right now? The PDF built fine for the accept PR, but it isn't appearing at https://doi.org/10.21105/joss.01531

@kyleniemeyer seems to appear for me fine on that link.

Weird! I still can't see it in Safari, but I can in Firefox. But the issue seems to be limited to me, so I'll say it's fine.

@ts-adi congrats on your article's publication in JOSS! Thanks to @dvberkel for reviewing and @gkthiruvathukal for editing!

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.01531/status.svg)](https://doi.org/10.21105/joss.01531)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01531">
  <img src="http://joss.theoj.org/papers/10.21105/joss.01531/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.01531/status.svg
   :target: https://doi.org/10.21105/joss.01531

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Awesome! Thanks for all your help with this @dvberkel @gkthiruvathukal @kyleniemeyer

A pleasure, @kyleniemeyer and @ts-adi. Big thanks to our reviewer, @dvberkel!

it was my pleasure. @ts-adi and team, you made awesome software

Was this page helpful?
0 / 5 - 0 ratings