[REVIEW]: SenSARP: A Python package for pre-processing Sentinel-1 SLC data with the Sentinel-1 Toolbox

whedon commented 3 years ago

Submitting author: @McWhity (Thomas Weiß) Repository: https://github.com/multiply-org/sar-pre-processing.git Version: v0.2 Editor: @pdebuyl Reviewers: @arthur-e, @danclewley Archive: 10.5281/zenodo.5903483

: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 badge code:

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

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

@arthur-e & @braunfuss, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
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 @pdebuyl 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 @arthur-e

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS 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] Contribution and authorship: Has the submitting author (@McWhity) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

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 functionality 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] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @danclewley

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS 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] Contribution and authorship: Has the submitting author (@McWhity) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[ ] Installation: Does installation proceed as outlined in the documentation?
[ ] Functionality: Have the functional claims of the software been confirmed?
[ ] 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 functionality 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] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @pdebuyl

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

whedon commented 3 years ago

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

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

watching

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

Failed to discover a Statement of need section in paper

whedon commented 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.09 s (531.8 files/s, 54898.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
XML                              4              7              0            983
reStructuredText                11            253             80            820
Python                          10            257            347            813
YAML                            10             39             47            314
make                             1             28              6            143
Markdown                         3             40              0            107
Jupyter Notebook                 1              0            274             75
TeX                              1              8              0             73
Bourne Shell                     2              1              0             24
HTML                             1              1              0              6
DOS Batch                        1              0              0              2
JSON                             1              0              0              1
-------------------------------------------------------------------------------
SUM:                            46            634            754           3361
-------------------------------------------------------------------------------

Statistical information for the repository '0325dfefdfd695f9447bbdb2' was
gathered on 2021/06/07.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
McWhity                         47          3004           1755           67.12
Thomas Weiß                     62           457            432           12.54
Tonio Fincke                    39           764            634           19.72
TonioF                           9            27             17            0.62

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Thomas Weiß                1114          243.8         30.1               23.52
Tonio Fincke                303           39.7         16.1                4.62

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

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3390/rs10081286 is OK
- 10.3390/w11091938 is OK
- 10.3390/rs12183037 is OK

MISSING DOIs

- None

INVALID DOIs

- None

pdebuyl commented 3 years ago

@arthur-e @braunfuss make sure to accept the invitation to the reviewers group and to have a look at the reviewer guidelines linked to at the top of this review page.

@McWhity JOSS requires that a section of the article is literally titled "statement of need", can you edit the article accordingly?

The review process will happen in this issue page, so questions to the author or to me can be added as comments here. As this is the first JOSS review for both reviewers, do not hesitate to ask questions if you have doubts about the procedure. You can also take a look at earlier reviews to get an idea of how the reviews proceed https://github.com/openjournals/joss-reviews/issues?q=is%3Aissue+label%3Aaccepted+

pdebuyl commented 3 years ago

@arthur-e @braunfuss do you need any assistance for starting the review ?

arthur-e commented 3 years ago

@pdebuyl I think I have everything I need, thanks!

whedon commented 3 years ago

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

whedon commented 3 years ago

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

arthur-e commented 3 years ago

I have completed my review.

I am unsure whether the submission requires "Minor" or "Major" revisions. I would tentatively propose that "Major revisions" are required only because I could not fully evaluate the functionality of the software due to an outstanding technical issue.

Comments to Authors

Overall, SenSARP seems to provide a valuable, very high-level processing chain for Sentinel 1 SLC data. However, there are two main areas in need of improvement: 1) Key functionality issue(s) need to be resolved; and, 2) The documentation should be improved and expanded to provide more details to users that may want to deviate from the prescribed pre-processing chain.

First, there is an unresolved issue as to the functionality of pre-processing Step 1. While unresolved, this issue prevents the software from functioning and, therefore, impedes further evaluation of its functionality.

I'd like to see the Documentation updated to provide end-users with more information on how to customize their processing chain. In addition to adding more details on the end-user functionality, there are a number syntax/ typographical issues with the documentation (e.g., missing citations in the ReadTheDocs compilation) that should fixed. Also, while SenSARP seems to have automated tests (and CI through Travis CI), the authors should add instructions to the README on how users can run tests themselves. I was able to run the two Python scripts in the tests/ directory without error, but I didn't get any feedback in the console so I can't be sure. I've written more about proposed Documentation improvements in the section on Documentation, below.

It is also difficult to determine how much the SenSARP Toolbox improves upon the ESA SNAP Sentinel-1 Toolbox. For instance, SARPreProcessor.pre_process_step1() consists of reading a configuration file, setting defaults, and using subprocess.call() to activate the corresponding SNAP Sentinel-1 Toolbox functions. The authors should revise the accompanying paper as well as the documentation to more explicitly state how this software improves upon the SNAP toolbox. If the major improvement is that SenSARP offers a programmatic API to the SNAP toolbox functions, that would be a meaningful contribution, but it should be made clear. While the authors do make the case that "an automatic or manual pre-processing of Sentinel-1 images is needed" (Lines 19-20), it would be good to know for sure that the SNAP toolbox does not offer such an automated pipeline, so that the value of SenSARP is obvious. In short, what are the relative advantages of SenSARP over the SNAP Toolbox, which is a dependency? I believe answering this question will better address the journal requirement: "A Statement of Need section that clearly illustrates the research purpose of the software." The journal recommends a section titled "Statement of Need" in the paper, though I think a section header is not necessary as long as the need is clearly described towards the beginning of the paper.

I think that SenSARP could be characterized differently in the JOSS paper and in the software's documentation/ README to make it clear that it provides a specific pre-processing chain with sensible(?) defaults--I would not call it a "toolbox" myself, because it has very few individual functions/ classes for end-users, but I do not dispute the use of that term. I think it might be more useful (and accurate?) to characterize SenSARP as a "push-button" or "turn-key" solution (or some other analogy) to automated pre-processing of Sentinel-1 SLC data. It seems that rather than a multi-functional programming API, SenSARP makes it easy to complete a complex but fairly rigid processing chain to multiple images. My first impression of SenSARP was that it would have multiple end-points/ functions/ classes for representing different kinds of data and processing steps; but, it seems to consist primarily of three end-user functions, pre_process_step1(), pre_process_step2(), pre_process_step3(). So, I think of SenSARP more as a pipeline rather than a "toolbox."

Finally, the authors should address the journal requirement on Community Guidelines: "There should be clear guidelines for third-parties wishing to: Contribute to the software; Report issues or problems with the software; Seek support." Some repositories include a CONTRIBUTING (Markdown or RST) file with detailed instructions about how Pull Requests and Issues should be formatted. Alternatively, this information could be added to a section of the README.

Functionality

As mentioned above, there is an unresolved issue as to the functionality of pre-processing Step 1.

Another outstanding issue that is unrelated may be a problem with the ESA data service and not with SenSARP. However, this issue (with the ESA data service) seems to severely impact part of the SenSARP functionality, in that data must be downloaded manually from somewhere else. The authors should address this shortcoming and provide some guidance to end-users in the Documentation and the Jupyter Notebook. Is there somewhere else users can get Sentinel 1 SLC data? I used NASA Earth Data Search.

Regarding Installation...

One key change that is needed: Version details for the SNAP Toolbox dependency. In the review, it became apparent that Version 8.0.4 (or greater?) is required but in the installation instructions there is no such information--and an unsupported version will be installed by default when the SNAP Toolbox is downloaded. Users should be told explicitly (how) to upgrade to 8.0.4.
I would prefer it if there were some way to install the software without using conda, which can be a pain to work with (and very slow). In this case, conda took several minutes to install the repository (spent a lot of time on "Solving environment"). Ideally, the setup.py script would list the package dependencies so that the repository (and its dependencies) could be installed with either distutils or pip. This can still be done within a conda virtual environment or, better yet, a virtualenv environment.
It would be helpful if the installation instructions could describe/ link to conda installation instructions, both for Anaconda and miniconda.
A few more details on how to download and install the ESA SNAP Sentinel toolbox would be helpful to users.

Documentation

In the "Usage" section, there seem to be some errors that occurred at the time the documentation was compiled (e.g., ModuleNotFoundError: No module named 'sentinelsat'). Fixing these and showing the correct output should help users to understand what the correct behavior of the software looks like.

I really like the detailed description of each processing step, however, the "Practical implementation" sections seem incomplete. While most users might use the defaults for pre_process_step1(), pre_process_step2(), and pre_process_step3(), is it possible that a user might want to skip or change a step? For instance, maybe a user wants to skip radiometric correct in this pipeline and do it differently later. Backscatter normalization is also "optional" but it's not clear from the documentation how to turn it on/ off. I would like to see these details fully described in the "Practical implementation" sections, perhaps with a minimal example (of how to configure the YAML file for that aim?).

There are also some minor syntax issues with the documentation that could be fixed, e.g., the Theory/purpose section of the section on TOPSAR Deburst reads: "More detailed information are provided in [], [] and []." I think that none of the in-line citations are appearing on ReadTheDocs for any section.

Finally, I think the README could be updated to provide explicit instructions to users for running the tests, including information as to how to evaluate they ran correctly--for instance, the users should see no output in the console if the tests succeeded, correct?

Software Paper

As discussed above, I believe the authors could do more to distinguish SenSARP from the SNAP Toolbox. Specifically:

How does SenSARP improve uponthe SNAP Toolbox? It would appear that SenSARP provides the pre-processing functionality of the SNAP Toolbox via subprocess calls to SNAP. This may be a substantial improvement over the graphical user interface (GUI) of SNAP through the ability to automate these steps at scale. If this is the primary contribution of SenSARP, the authors should explicitly state so in the paper.
Is SenSARP a fairly rigid pre-processing pipeline or is it a multi-functionality API for creating customized GPT pipelines? Neither the documentation nor the Jupyter Notebook provide examples of how the pipeline might be customized to skip or change certain pre-processing steps.

pdebuyl commented 3 years ago

Hi @arthur-e thank you very much for the review. With JOSS, we do not expect a "minor/major revision" assessment. Providing a detailed report, as you have done, participates in the constructive review process which will take place in this issue.

arthur-e commented 3 years ago

@pdebuyl Thanks! The "Review criteria" may need to be updated as it still mentions "Minor" and "Major" revisions.

pdebuyl commented 3 years ago

Thank you @arthur-e for pointing this out. In practice, the reviews proceed by iterative improvements and I forgot that we request this grading. I'll discuss it with the editor group.

pdebuyl commented 3 years ago

@McWhity you can already address @arthur-e 's review (without waiting for @braunfuss ' one) or wait for the second review to arrive.

McWhity commented 3 years ago

@McWhity you can already address @arthur-e 's review (without waiting for @braunfuss ' one) or wait for the second review to arrive.

Yes I will address the review in the next view days

pdebuyl commented 3 years ago

Hi @braunfuss any timeline for the review?

McWhity commented 3 years ago

Unfortunately, I haven't found time to address the detailed review of @arthur-e yet. I think I will be able to update the repository based on the review of @arthur-e until 22nd of August. This means in terms of saving work time for the reviewers, @braunfuss might wait with conducting his review until the 22nd of August. If @braunfuss is almost finished with his review, I will be happy to include his remarks too.

pdebuyl commented 3 years ago

Hi @McWhity is the update coming as planned?

McWhity commented 3 years ago

Sorry, I'm a little bit behind schedule. I'm currently testing the implemented changes. Therefore, the new version should be ready within this week. I will keep you posted when I push the final changes. Furthermore, I will provide a detailed answer to the review comments.

McWhity commented 3 years ago

I have completed my review.

I am unsure whether the submission requires "Minor" or "Major" revisions. I would tentatively propose that "Major revisions" are required only because I could not fully evaluate the functionality of the software due to an outstanding technical issue.

Comments to Authors

Overall, SenSARP seems to provide a valuable, very high-level processing chain for Sentinel 1 SLC data. However, there are two main areas in need of improvement: 1) Key functionality issue(s) need to be resolved; and, 2) The documentation should be improved and expanded to provide more details to users that may want to deviate from the prescribed pre-processing chain.

First, there is an unresolved issue as to the functionality of pre-processing Step 1. While unresolved, this issue prevents the software from functioning and, therefore, impedes further evaluation of its functionality.

I'd like to see the Documentation updated to provide end-users with more information on how to customize their processing chain. In addition to adding more details on the end-user functionality, there are a number syntax/ typographical issues with the documentation (e.g., missing citations in the ReadTheDocs compilation) that should fixed. Also, while SenSARP seems to have automated tests (and CI through Travis CI), the authors should add instructions to the README on how users can run tests themselves. I was able to run the two Python scripts in the tests/ directory without error, but I didn't get any feedback in the console so I can't be sure. I've written more about proposed Documentation improvements in the section on Documentation, below.

Thank you for pointing this out.

Syntax/typographical issues (missing citations, ReadTheDocs compilation) within documentation are resolved

Documentation is updated to provide more end-user information (see answers to review points on the documentation)

Information how to run test within Readme and ReadtheDocs documentation were added

It is also difficult to determine how much the SenSARP Toolbox improves upon the ESA SNAP Sentinel-1 Toolbox. For instance, SARPreProcessor.pre_process_step1() consists of reading a configuration file, setting defaults, and using subprocess.call() to activate the corresponding SNAP Sentinel-1 Toolbox functions. The authors should revise the accompanying paper as well as the documentation to more explicitly state how this software improves upon the SNAP toolbox. If the major improvement is that SenSARP offers a programmatic API to the SNAP toolbox functions, that would be a meaningful contribution, but it should be made clear. While the authors do make the case that "an automatic or manual pre-processing of Sentinel-1 images is needed" (Lines 19-20), it would be good to know for sure that the SNAP toolbox does not offer such an automated pipeline, so that the value of SenSARP is obvious. In short, what are the relative advantages of SenSARP over the SNAP Toolbox, which is a dependency? I believe answering this question will better address the journal requirement: "A Statement of Need section that clearly illustrates the research purpose of the software." The journal recommends a section titled "Statement of Need" in the paper, though I think a section header is not necessary as long as the need is clearly described towards the beginning of the paper.

I think that SenSARP could be characterized differently in the JOSS paper and in the software's documentation/ README to make it clear that it provides a specific pre-processing chain with sensible(?) defaults--I would not call it a "toolbox" myself, because it has very few individual functions/ classes for end-users, but I do not dispute the use of that term. I think it might be more useful (and accurate?) to characterize SenSARP as a "push-button" or "turn-key" solution (or some other analogy) to automated pre-processing of Sentinel-1 SLC data. It seems that rather than a multi-functional programming API, SenSARP makes it easy to complete a complex but fairly rigid processing chain to multiple images. My first impression of SenSARP was that it would have multiple end-points/ functions/ classes for representing different kinds of data and processing steps; but, it seems to consist primarily of three end-user functions, pre_process_step1(), pre_process_step2(), pre_process_step3(). So, I think of SenSARP more as a pipeline rather than a "toolbox."

Thank you for these very helpful remarks. A section "Statement of Need" addressing the mentioned shortcomings is now added to the Paper, the Readme, and the Documentation. SenSARP is not characterized as "toolbox" anymore but as suggested SenSARP is now called a "push-button" / "pipeline" solution

Finally, the authors should address the journal requirement on Community Guidelines: "There should be clear guidelines for third-parties wishing to: Contribute to the software; Report issues or problems with the software; Seek support." Some repositories include a CONTRIBUTING (Markdown or RST) file with detailed instructions about how Pull Requests and Issues should be formatted. Alternatively, this information could be added to a section of the README.

Thank you for pointing this out. A section "Support, contributing and testing" was added to the Readme and the documentation

Functionality

As mentioned above, there is an unresolved issue as to the functionality of pre-processing Step 1.

Please see answer to an unresolved issue

Another outstanding issue that is unrelated may be a problem with the ESA data service and not with SenSARP. However, this issue (with the ESA data service) seems to severely impact part of the SenSARP functionality, in that data must be downloaded manually from somewhere else. The authors should address this shortcoming and provide some guidance to end-users in the Documentation and the Jupyter Notebook. Is there somewhere else users can get Sentinel 1 SLC data? I used NASA Earth Data Search.

Thank you for this remark. You are right SenSARP can only be used if the Sentinel-1 data is available. Two options are now introduced within the jupyter notebook.

Option 1: Downloading data manually or via sentinelsat from Copernicus data hub.

Option 2: Downloading data from the Alaska Satellite Facility (ASF uses the same credentials als NASA Earth Data Search). The two options are also introduced within the documentation

Regarding Installation...
* **One key change that is needed: Version details for the SNAP Toolbox dependency.** In the review, [it became apparent that Version 8.0.4 (or greater?) is required](https://github.com/multiply-org/sar-pre-processing/issues/12#issuecomment-863323367) but in the installation instructions there is no such information--and an unsupported version will be installed by default when the SNAP Toolbox is downloaded. Users should be told explicitly (how) to upgrade to 8.0.4.
Thank you for this comment. Information that a update is needed as well as information how to update SNAP is given within the Readme, the beginning of the jupyter notebook and the installation part of the documentation
* I would prefer it if there were some way to install the software without using `conda`, which can be a pain to work with (and very slow). In this case, `conda` took several minutes to install the repository (spent a lot of time on "Solving environment"). Ideally, the `setup.py` script would list the package dependencies so that the repository (and its dependencies) could be installed with either `distutils` or `pip`. This can still be done within a `conda` virtual environment or, better yet, a `virtualenv` environment.

* It would be helpful if the installation instructions could describe/ link to `conda` installation instructions, both for Anaconda and `miniconda`.

* A few more details on how to download and install the ESA SNAP Sentinel toolbox would be helpful to users.

Thank you for this remark.

Instruction how to install SenSARP via virtualenv and python were added. As the installation of GDAL can be very tricky, GDAL needs to be installed separately. Instruction how to do this is were added
Information how to download and install Anaconda and Miniconda were added
More details on how to download and install the ESA SNAP Sentinel toolbox were added

Please see installation part of readme and documentation

Documentation

In the "Usage" section, there seem to be some errors that occurred at the time the documentation was compiled (e.g., ModuleNotFoundError: No module named 'sentinelsat'). Fixing these and showing the correct output should help users to understand what the correct behavior of the software looks like.

Thank you for pointing this out. The "Usage" section was revised. Now showing three jupyter notebook examples (default processing of a single image, default processing a time series, processing a time series with a user defined xml graph) and an explanation of the config file.

I really like the detailed description of each processing step, however, the "Practical implementation" sections seem incomplete. While most users might use the defaults for pre_process_step1(), pre_process_step2(), and pre_process_step3(), is it possible that a user might want to skip or change a step? For instance, maybe a user wants to skip radiometric correct in this pipeline and do it differently later. Backscatter normalization is also "optional" but it's not clear from the documentation how to turn it on/ off. I would like to see these details fully described in the "Practical implementation" sections, perhaps with a minimal example (of how to configure the YAML file for that aim?).

Thank you for pointing this out. As the theory of default pre-processing steps are explained within "Practical implementation" the section was renamed to "Default processing chain". For non-expert users skipping of a step is not intended right now. I understand the confusion about the "optional" behind the "backscatter normalization" step. The backscatter normalization as well as the multi-temporal filter are applied in any case. However, the final netcdf file contains layers of

single speckle filtered radiometric and geometric corrected sigma nought backscatter

multi speckle filtered radiometric and geometric corrected sigma nought backscatter

single speckle filtered radiometric and geometric corrected sigma nought backscatter normalized to a specific incidence angle

multi speckle filtered radiometric and geometric corrected sigma nought backscatter normalized to a specific incidence angle

The missing information about which layers and which content is provided by the final netcdf file a section explaining the output layers was added. Another section "Folder and data creation during pre-processing steps" explaining which folders are created during the default pre-processing was added as well. Regarding the skipping of specific processing steps. A new jupyter notebook explaining how expert users can modify the default xml-processing graphs or create their own processing graphs and use the SenSARP functionalities was added to the "Usage" section.

There are also some minor syntax issues with the documentation that could be fixed, e.g., the Theory/purpose section of the section on TOPSAR Deburst reads: "More detailed information are provided in [], [] and []." I think that none of the in-line citations are appearing on ReadTheDocs for any section.

Thank you for pointing this out. Syntax issues were fixed.

Finally, I think the README could be updated to provide explicit instructions to users for running the tests, including information as to how to evaluate they ran correctly--for instance, the users should see no output in the console if the tests succeeded, correct?

Thank you for pointing this out. As mentioned previously a section "Support, contributing and testing" was added. If you run "pytest" in the terminal you should receive an output if the test did run correctly or if they failed.

Software Paper

As discussed above, I believe the authors could do more to distinguish SenSARP from the SNAP Toolbox. Specifically:

* How does SenSARP improve uponthe SNAP Toolbox? It would appear that SenSARP provides [the pre-processing functionality of the SNAP Toolbox](http://step.esa.int/main/toolboxes/sentinel-1-toolbox/s1tbx-features/) via `subprocess` calls to SNAP. This may be a substantial improvement over the graphical user interface (GUI) of SNAP through the ability to automate these steps at scale. If this is the primary contribution of SenSARP, the authors should explicitly state so in the paper.

Thank you for your very helpful remarks. As already mentioned a section "Statement of need" clarifying the mentioned shortcomings was added. Another feedback in this matter is highly appreciated.

* Is SenSARP a fairly rigid pre-processing pipeline or is it a multi-functionality API for creating customized GPT pipelines? Neither the documentation nor the Jupyter Notebook provide examples of how the pipeline might be customized to skip or change certain pre-processing steps.

Thank you for your very helpful remarks. A new juypter notebook explaining how expert users can customize or use their own pre-processing chain was added.

pdebuyl commented 3 years ago

Hi @McWhity , sorry I had missed your replies. Thank you for addressing the comments by @arthur-e .

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

pdebuyl commented 3 years ago

Hi @McWhity for information, I have started looking for an alternative second reviewer.

McWhity commented 3 years ago

Thank you for the update

pdebuyl commented 3 years ago

@whedon add @danclewley as reviewer

whedon commented 3 years ago

OK, @danclewley is now a reviewer

pdebuyl commented 3 years ago

@whedon remove @braunfuss as reviewer

whedon commented 3 years ago

OK, @braunfuss is no longer a reviewer

pdebuyl commented 3 years ago

Hi @danclewley thank you for accepting to review. The general guidelines for reviewing are here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html

In a nutshell:

Read our review criteria.
Take a look at the package, install it, check and run the tests and examples.
Go over your checklist (in the first message of this page) and use it as a guide to perform the review.

Don't forget to accept the invitation to the group of reviewers, you need to accept it to be able to check the items in the checklist.

pdebuyl commented 3 years ago

@McWhity for information, @danclewley told me in advance that the review would start by the end of october. Sorry for the extra delay to your submission.

danclewley commented 3 years ago

I'm working my way through the review, still need to get the software installed and test it.

Some general comments so far:

While this is a nice and well documented library, that looks like it will make processing Sentinel 1 data using SNAP easier for some users I'm struggling to see who it is aimed at from the paper and how it represents 'Substantial scholarly effort' (as per journal guidelines).

In the Statement of need it is mentioned 'SenSARP was developed to provide a push-button option to easily apply a rigid pre-processing pipeline with sensible defaults'.

Knowing which steps and options to use in SNAP to give the best results is quite difficult, so if the authors have taken this work out for users then this should be mentioned. Also the settings will depend on the application, if the settings have been optimised for a particular application (e.g., land cover change) then this would be good to state, again makes it easier for users. Related to this, I thought the following page in the documentation which the theory and practical implementation of each of the stages was really nice: https://multiply-sar-pre-processing.readthedocs.io/en/latest/04_Theory.html

There are also some other libraries which I think there is some overlap with which aren't mentioned:

https://snap-contrib.github.io/snapista/ - Python interface for SNAP GPF (more general though not targeted at SAR users)
https://pyrosar.readthedocs.io/en/latest/index.html - This looks like it does a very similar job (processing S1 data using SNAP), I've only found out about this recently so the authors might not be aware of it.

I'll finish my review and provide my recommendation once I've managed to install and test the software.

pdebuyl commented 3 years ago

Hi @danclewley thank you for getting on to this.

Regarding scholarly efforts, you can find the criteria here: https://joss.readthedocs.io/en/latest/review_criteria.html#substantial-scholarly-effort

During the pre-review phase, during which the question was raised by Daniel Katz, the author already provided extra information https://github.com/openjournals/joss-reviews/issues/3198#issuecomment-841809933

Among the criteria for scholary effort, the paper mentions existing applications of the package, which also count toward the evalution. I let the author reply for further motivation if necessary.

danclewley commented 3 years ago

Apologies this has taken so long.

I'm happy to recommend accept with minor revisions if the paper is updated to clarify who it is aimed at and how it differs from other packages.

pdebuyl commented 3 years ago

Hi @danclewley thank for the feedback.

To proceed, I would like to ask you to fill the checklist for all items that you agree on. This is part of the JOSS review process and it in general also a nice way for the authors to track the progress of the review. To accept the paper, once you are ok, I would also like an explicit statement of acceptance here. (This is not part of JOSS guidelines but without this, with the back and forth messages and the absence of notifications for checklist editing, I find it harder to conclude the review).

pdebuyl commented 3 years ago

@arthur-e would you update your checklist as well in light of the updates?

arthur-e commented 2 years ago

@pdebuyl I am satisfied that the authors have revised the manuscript, the software, and the documentation to meet the journal's standards. I have updated my checklist to reflect that.

pdebuyl commented 2 years ago

Thanks a lot for finishing the review @arthur-e

pdebuyl commented 2 years ago

Hi @McWhity can you reply to @danclewley 's review?

McWhity commented 2 years ago

Sorry for the delay. I will give an update/response on the weekend.

@arthur-e and @danclewley: Thanks a lot for the review.

McWhity commented 2 years ago

Apologies this has taken so long.

I'm happy to recommend accept with minor revisions if the paper is updated to clarify who it is aimed at and how it differs from other packages.

@danclewley: Thank you for the review. I updated the paper (https://github.com/multiply-org/sar-pre-processing/blob/master/docs/paper.pdf).

Main changes are:

last paragraph of method section was rewritten:

After the pre-processing the resulting radiometrically and geometrically corrected images are stored for further usage within a NetCDF4 stack file. The processing workflow was developed and optimized to use a Sentinel-1 time series of pre-processed sigma nought backscatter values to retrieve biophysical land surface parameters by the use of radiative transfer models. The sigma nought backscatter values provided by the default workflow of SenSARP might be used in other applications like flood risk analysis, monitoring land cover changes or monitoring global food security but it has to be mentioned that different applications have different demands and therefore, slight adjustments of the default workflow might be required. In the future, many more new products and operational third party services based on consistent Sentinel-1 time series might be developed._
new Section regarding other python packages was added:

Other available python software packages using ESA's SNAP software to pre-process SAR data The ESA's SNAP toolbox has been written in Java. For python users the developers provide a python interface called Snappy. However, the Snappy interface is lacking in terms of installation, processing performance and usability. Hence, the remote sensing community developed different wrappers (e.g. SenSARP, snapista or pyroSAR) to use SNAP processing functionalities by utilizing the SNAP Graph Processing Tool (GPT).

snapista Snapista (https://snap-contrib.github.io/snapista/index.html) targets mainly experts remote sensing users with python programming skills. It provides access to the processing operators of all toolboxes (e.g. Sentinel-1, Sentinel-2 or Sentinel-3) within SNAP. Expert users can generate processing graphs and execute there generated graphs in a pure Pythonic way. A guideline which processing steps are needed for different applications or which processing steps can or have to be combined are not provided yet. As guidelines how to process different satellite data for different applications is not an easy task to do it exceeds the goal of snapista as a python wrapper for the SNAP software. Summarizing, snapista provides access to all SNAP toolboxes (not just to Sentinel-1 Toolbox) via python. But as it provides no default processing chains, snapista will be primarily usable by expert remote sensing users. The advantage of snapista is the accessibility of processing operators for SAR and optical data.

pyroSAR PyroSAR (https://pyrosar.readthedocs.io/en/latest/index.html) is a python library which provides a python wrapper to SAR pre-processing software SNAP and GAMMA [@wegnuller_sentinel-1_2016; @werner_gamma_2000]. The library provides utilities to read and store metadata information of downloaded satellite data within a database. Furthermore, pyroSAR provides access to processing operators of SNAP and GAMMA. A default workflow with different user options is provided to process single or time-series Sentinel-1 images. After executing the default processing workflow radiometric and geometric corrected gamma nought backscatter data are provided in Geotiff format [@truckenbrodt_pyrosar_2019]. The processed images can also be stored within an Open Data Cube. For expert users which might want to use a different processing workflow pyroSAR provides an option to create SNAP xml-workflows and execute them with the GPT. Summarizing, pyroSAR provides a similar push-button option to process Sentinel-1 data with a slightly different default workflow (pyroSAR: no temporal speckle filter, gamma nought backscatter output in Geotiff format) than SenSARP (SenSARP: temporal speckle filter, sigma nought backscatter output in netCDF format). PyroSAR, as a more complex library than SenSARP, provides on the one hand more changeable parameters within the processing workflow but on the other hand the usability for non-expert users might be narrowed compared to SenSARP. An advantage of SenSARP, especially for non-expert users, might be the provision of background information (theory/purpose) of the different pre-processing steps within the documentation.

I hope these changes satisfying your request

pdebuyl commented 2 years ago

Thank you @McWhity for the update.

pdebuyl commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

pdebuyl commented 2 years ago

@danclewley can you take @McWhity 's update in your review? Please make sure to tick the appropriate checkboxes at the top of this page as appropriate.

danclewley commented 2 years ago

Updates look good to me and address my comments. Happy to recommend accepting.

pdebuyl commented 2 years ago

Hi @danclewley thanks for the feedback! Can you check all relevant checkboxes? This is part of the JOSS process and despite your message I don't think it would be right for me to check them :-)

pdebuyl commented 2 years ago

Thanks @danclewley for filling in the form.

pdebuyl commented 2 years ago

Hi @McWhity the review is almost done, but as there was some issue for installing it for @danclewley I am checking with the board for a way out, by inviting a third reviewer for these steps for instance. I'll keep you updated.

openjournals / joss-reviews