search

openjournals / joss-reviews

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

[REVIEW]: SAR tools: A QGIS plugin for generating SAR descriptors #2970

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @Narayana-Rao (NR Bhogapurapu) Repository: https://github.com/Narayana-Rao/SAR-tools Version: v0.7 Editor: @hugoledoux Reviewer: @liberostelios, @HenrikJanPersson Archive: 10.5281/zenodo.4621292

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

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

@liberostelios, 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 @hugoledoux 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 @liberostelios

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @HenrikJanPersson

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. @liberostelios 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

Downloading of the repository (to check the bibtex) failed for issue #2970 failed with the following error:

/app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/response/raise_error.rb:16:in on_complete': GET https://api.github.com/repos/openjournals/joss-reviews/issues/2970: 404 - Not Found // See: https://docs.github.com/rest/reference/issues#get-an-issue (Octokit::NotFound) from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/response.rb:9:inblock in call' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/response.rb:61:in on_complete' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/response.rb:8:incall' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/middleware/follow_redirects.rb:73:in perform_with_redirection' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/middleware/follow_redirects.rb:61:incall' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/rack_builder.rb:143:in build_response' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/connection.rb:387:inrun_request' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/connection.rb:138:in get' from /app/vendor/bundle/ruby/2.6.0/gems/sawyer-0.8.2/lib/sawyer/agent.rb:94:incall' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/connection.rb:156:in request' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/connection.rb:19:inget' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/client/issues.rb:114:in issue' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/review.rb:21:inissue_body' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:368:in review_issue' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:378:indownload' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:38:in download' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:131:in<top (required)>' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in

'

whedon commented 3 years ago

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

/app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/response/raise_error.rb:16:in on_complete': GET https://api.github.com/repos/openjournals/joss-reviews/issues/2970: 404 - Not Found // See: https://docs.github.com/rest/reference/issues#get-an-issue (Octokit::NotFound) from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/response.rb:9:inblock in call' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/response.rb:61:in on_complete' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/response.rb:8:incall' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/middleware/follow_redirects.rb:73:in perform_with_redirection' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/middleware/follow_redirects.rb:61:incall' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/rack_builder.rb:143:in build_response' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/connection.rb:387:inrun_request' from /app/vendor/bundle/ruby/2.6.0/gems/faraday-0.15.4/lib/faraday/connection.rb:138:in get' from /app/vendor/bundle/ruby/2.6.0/gems/sawyer-0.8.2/lib/sawyer/agent.rb:94:incall' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/connection.rb:156:in request' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/connection.rb:19:inget' from /app/vendor/bundle/ruby/2.6.0/gems/octokit-4.8.0/lib/octokit/client/issues.rb:114:in issue' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/review.rb:21:inissue_body' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:44:in prepare' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:131:in<top (required)>' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in

'

Narayana-Rao commented 3 years ago

@whedon generate pdf

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:

whedon commented 3 years ago

:wave: @liberostelios, please update us on how your review is going.

hugoledoux commented 3 years ago

@whedon re-invite @HenrikJanPersson as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@henrikjanpersson please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

HenrikJanPersson commented 3 years ago

The plugin is clean and has a well defined purpose. The paper is short but well written, but it is missing several required parts. Additionally, please see my comments below.

  1. The purpose is to provide an easy procedure to compute a number of polarimetric indices (parameters) that may be of interest for the user. However, it requires the input data in a format that SNAP and PolSAR Pro delivers, and this SW combination already provides the functionality of deriving the indices that this plugin computes. Therefore, I am still wondering about the need for this plugin?
  2. An example of how the PI is used is still missing. Suggest to also tell the user about the sample data.
  3. I suggest changing the name from SAR Tools to PolSAR Tools, since they are related to polarimetric SAR, not SAR generally.

Some minor suggestions for improving the PI are listed here:

  1. change the name of "View data" to "Select data" or "Open file".
  2. provide an information dialog about what happened when the files were generated (where they are located, and possibly also how to open them).
  3. add option for selecting the output folder.
  4. process bar not updating when computing an index.
  5. why is it necessary to both select a folder and a file, independent of each other? Insufficient error message to user if they are not matching.
  6. It is not possible to select the "folder" (for whatever use this is?), without first selecting an index. Would be helpful to provide this as information (in the gray folder path area) until an index is selected.
  7. cannot change to T-matrix from the pre-selected C-matrix.
  8. no information about what matrix that is expected (e.g., T3/C3) if wrong file is selected
hugoledoux commented 3 years ago

The paper is short but well written, but it is missing several required parts.

Just to clarify: the paper is supposed to be short in JOSS, but indeed the points 1 and 2 you bring should be in the paper. As for the new name, this could mean that repository name is changed and other side consequences, so I'll leave it to the authors PolSAR does sound better to me though (but I am not an expert in this field).

Narayana-Rao commented 3 years ago

Dear @hugoledoux and @HenrikJanPersson,

Thank you very much for your constructive comments. Please find the attached document for a detailed response to your comments.

Response_to_review_comments_2970.pdf

liberostelios commented 3 years ago

The paper is well written and quite concise. As a non-expert in the SAR processing field I can't state much about this aspect, although as a GIS user and developer I wonder to which extend there is an overlap between this tool and GRASS (whose processing toolkit can be used from inside QGIS). Do you think it would be useful to provide a simple statement about how GRASS can deal with SAR and how your plugin might relate to this (a sentence or two might suffice)?

The plugin is easy to install and seems to be working for the most part. However, here are some comments about it:

HenrikJanPersson commented 3 years ago

@whedon generate pdf

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:

HenrikJanPersson commented 3 years ago

Thank you for the updates and for constructive feedback on my comments. Several things have been clarified and I think the changes have improved the overall impression greatly. I still have a few important comments.

SAR tools or any other more descriptive name: I agree with the authors that PolSAR often covers quad-pol data. Yet, I disagree with the authors that PolSAR would be limited to this type. In my experience, Polarimetric SAR intends all types of SAR where the polarization matters and more than a single combination is considered (e.g., HH, VV, or HV). In particular, PolSAR covers the target decomposition the authors are well aware of and also describe in the paper and address with the plugin, therefore it would be a much better fit. The name "SAR tools" would let the user assume to also get access to basic SAR operations, e.g., multilooking and basic filtering operations, which currently is not the case. I think SAR tools is simply too generic for this very particular set of operations you provide. Assuming you stick with SARtools, I think the background in the paper must ensure to not only referring to free products powered by ESA. Be ware of other softwares too (not limited to PolSAR but SAR in general), like DORIS, ISCE, GMTSAR, ROI_PAC and more.

Paper: I would suggest to change "... all the three available ..." to only "... the three ...", since there are non-linear polarimetric acquisition modes as well (e.g. circular), which you do not cover.

Detail:

process bar not updating when computing an index.

The progress will get updated for the currently running process (Eg. Computing RVI from Full-pol data), however, if the dataset is small (like the sample data set), you may not notice the update because the whole progress bar will be fi lled and emptied in less than a second.

If it computes this so fast that I cannot notice, why not leaving it at 100% when finished? I see no point of resetting it to 0% before a new Process has been activated.

hugoledoux commented 3 years ago

@Narayana-Rao is it clear to you that the review is iterative? I am asking because you wrote a formal response to reviewer in a PDF, and while it's fine, I believe it would be better if you just answered here in plain text and discussed with the reviewers.

For the name, as I said, this is up to you, but as @HenrikJanPersson pointed out, it would be a better name and furthermore I reckon that since you're still in beta it's still time to change.

The comments of @liberostelios about the fact that the plugin is not cross-platform should definitively be addressed, and as for the GUI, I would also encourage you to use the "standard" QGIS one (it will also make is easier to upgrade the code in the future!).

Let us now which timetime you have in mind to address those issues.

Narayana-Rao commented 3 years ago

Dear @hugoledoux @HenrikJanPersson and @liberostelios, Thank you very much for your constructive comments. Please find the attached document for a detailed response to your comments.

Response_to_review_comments_2970_R2.pdf

hugoledoux commented 3 years ago

@Narayana-Rao Great that you changed the named 👍

For the GUI, I agree with @liberostelios that a processing interface would be better, but also realise that perhaps it is asking too much. However, I think you have to at least harmonise with other plugins, and provide an "OK/Run" button at the bottom right, and a "Cancel" button too. Right now there is no cancel button whatsoever. Also, the help should be a "Help" button at the bottom-left, like for virtually all plugins.

See for instance this one: IMAGE 2021-03-15 15:22:46

Your "process" button is not a button, and will create confusion in my opinion, see when I enlarge the window under macOS: CleanShot 2021-03-15 at 15 22 00@2x

@liberostelios & @HenrikJanPersson : some of the checkboxes at the top are not clicked yet. Is it because the author needs to add something? Or is it because you haven't had time yet to look at this?

liberostelios commented 3 years ago

I agree with @hugoledoux comments about GUI, but other than that I can raise any big objections on the publication of the paper. So I've filled all checkboxes from my side.

Narayana-Rao commented 3 years ago

Dear @hugoledoux, I want to thank you for positively responding to our "response to review comments" on behalf of our team. I appreciate your suggestions regarding the UI. I agree that our tool/plugin should have standard UI elements, which will enhance usability and avoid confusion that may arise due to a non-standard UI. Therefore, according to your suggestions, we have now updated the plugin UI in the recent release (v0.7). Additionally, we have added one more functionality (Full-pol --> MF4CF [1]) based on our recent research work.

[1] S. Dey, A. Bhattacharya, A. C. Frery, C. Lopez-Martinez and Y. S. Rao, "A Model-free Four Component Scattering Power Decomposition for Polarimetric SAR Data," in IEEE Journal of Selected Topics in Applied Earth Observations and Remote Sensing, 2021. doi: 10.1109/JSTARS.2021.3069299.

Narayana-Rao commented 3 years ago

I agree with @hugoledoux comments about GUI, but other than that I can raise any big objections on the publication of the paper. So I've filled all checkboxes from my side.

Dear @liberostelios, Thank you very much for accepting the paper on the PolSAR tools plugin. Your comments on the plugin helped us a lot in improving the overall quality of the plugin.

hugoledoux commented 3 years ago

👋 @HenrikJanPersson we are waiting for you to confirm whether the missing checkboxes are an oversight, or if you expect the authors to fix more things?

hugoledoux commented 3 years ago

@whedon re-invite @HenrikJanPersson as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@henrikjanpersson please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

HenrikJanPersson commented 3 years ago

@whedon generate pdf

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:

HenrikJanPersson commented 3 years ago

@Narayana-Rao Thank you for the changes. I think it all looks fine and that the tool and paper make a good contribution! I have checked all boxes.

Narayana-Rao commented 3 years ago

@Narayana-Rao Thank you for the changes. I think it all looks fine and that the tool and paper make a good contribution! I have checked all boxes.

Dear @HenrikJanPersson , We want to thank you for your valuable time and suggestions, which has been instrumental in improving the technical quality of the plugin as well as the manuscript.

Narayana-Rao commented 3 years ago

@whedon generate pdf

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:

hugoledoux commented 3 years ago

OK, we're getting to the end of this review.

I went over the paper and fixed a few things, could you check it and merge (if you agree): https://github.com/Narayana-Rao/PolSAR-tools/pull/7

Narayana-Rao commented 3 years ago

@whedon generate pdf

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:

Narayana-Rao commented 3 years ago

OK, we're getting to the end of this review.

I went over the paper and fixed a few things, could you check it and merge (if you agree): Narayana-Rao/PolSAR-tools#7

Dear @hugoledoux, Thank you very much for your edits. I have checked and merged your pull request Narayana-Rao/PolSAR-tools#7.

hugoledoux commented 3 years ago

@whedon generate pdf

hugoledoux 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.1109/JSTARS.2021.3069299 is OK
- 10.1016/j.rse.2020.111954 is OK
- 10.1016/j.jag.2020.102052 is OK
- 10.1109/tgrs.2020.2976661 is OK
- 10.1016/j.isprsjprs.2020.09.010 is OK
- 10.1109/lgrs.2019.2907703 is OK
- 10.1109/tgrs.2018.2848285 is OK
- 10.1109/TGRS.2009.2014944 is OK
- 10.1016/0030-4018(77)90292-9 is OK

MISSING DOIs

- None

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:

hugoledoux commented 3 years ago

OK, both reviewers recommend acceptance so we're moving towards this.

At this point could you:

I can then move forward with accepting the submission.

Narayana-Rao commented 3 years ago

OK, both reviewers recommend acceptance so we're moving towards this.

At this point could you:

  • [ ] Make a tagged release of your software, and list the version tag of the archived version here.
  • [ ] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
  • [ ] Check the archival deposit (e.g., in Zenodo) 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.
  • [ ] Please list the DOI of the archived version here.

I can then move forward with accepting the submission.

Dear @hugoledoux,

I have finished the archival of the repository according to the given guidelines. Please verify and confirm the following check list.

  1. Version v0.7
  2. Zenodo archive 10.5281/zenodo.4621292
  3. I have verified the archive's metadata and also added ORCIDs of all the authors.
  4. Zenodo archive 10.5281/zenodo.4621292
hugoledoux commented 3 years ago

the title in zenodo is not the same as your paper, this is the only thing left at this point, could you please change it?

oh and your name is also spelled differently, shouldn't they both be the same?

Narayana-Rao commented 3 years ago

the title in zenodo is not the same as your paper, this is the only thing left at this point, could you please change it?

oh and your name is also spelled differently, shouldn't they both be the same?

Dear @hugoledoux, Thank you for pointing out these. I have updated them now. Please check and confirm. 10.5281/zenodo.4621292

hugoledoux commented 3 years ago

@whedon set 10.5281/zenodo.4621292 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4621292 is the archive.

hugoledoux commented 3 years ago

@whedon set v0.7 as version

whedon commented 3 years ago

OK. v0.7 is the version.

hugoledoux commented 3 years ago

@whedon accept

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

OK DOIs

- 10.1109/JSTARS.2021.3069299 is OK
- 10.1016/j.rse.2020.111954 is OK
- 10.1016/j.jag.2020.102052 is OK
- 10.1109/tgrs.2020.2976661 is OK
- 10.1016/j.isprsjprs.2020.09.010 is OK
- 10.1109/lgrs.2019.2907703 is OK
- 10.1109/tgrs.2018.2848285 is OK
- 10.1109/TGRS.2009.2014944 is OK
- 10.1016/0030-4018(77)90292-9 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 3 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/2208

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

@whedon accept deposit=true