openjournals / joss-reviews

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

[REVIEW]: PyFlowline a mesh independent river network generator for hydrologic models #5446

Closed editorialbot closed 10 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@changliao1025<!--end-author-handle-- (Chang Liao) Repository: https://github.com/changliao1025/pyflowline Branch with paper.md (empty if default branch): main Version: v0.3.4 Editor: !--editor-->@observingClouds<!--end-editor-- Reviewers: @smchartrand, @andres-patrignani Archive: 10.5281/zenodo.10076553

Status

status

Status badge code:

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

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

@smchartrand & @andres-patrignani, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @observingClouds know.

Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest

Checklists

📝 Checklist for @smchartrand

📝 Checklist for @andres-patrignani

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

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

OK DOIs

- 10.5281/zenodo.5558988 is OK
- 10.5194/hess-26-5473-2022 is OK
- 10.1029/2022MS003089 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=1.33 s (99.6 files/s, 337480.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JSON                            22              2              0         420104
Python                          69           1982           1794           8325
C++                              1            331           1115           6043
C                                1            328           1093           5936
reStructuredText                19            309             65            683
Cython                           2            100             63            262
DOS Batch                        2             37              2            238
make                             2             49              6            209
Markdown                         6             87              0            109
YAML                             5             15             28             77
Jupyter Notebook                 1              0            978             67
TeX                              1              3              0             35
INI                              1              4              0             13
TOML                             1              0              0              6
-------------------------------------------------------------------------------
SUM:                           133           3247           5144         442107
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 743

editorialbot commented 1 year ago

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

observingClouds commented 1 year ago

Hi @smchartrand, @andres-patrignani, I just want to check if you have any questions regarding the review process. As a start, each of you should run @editorialbot generate my checklist to get a checklist of the individual tasks a JOSS review requires. Tasks can then be ticked off one by one. You can find additional information in the guidelines. Also, feel free to tag me here if you have any additional questions.

Cheers!

smchartrand commented 1 year ago

Review checklist for @smchartrand

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

observingClouds commented 1 year ago

Hi everyone, I'm glad to see that the review process is in full swing now and the first issues are being created. @andres-patrignani if you could run editorialbot generate my checklist as well that would be awesome. It makes it easier for me to track the process.

smchartrand commented 1 year ago

Hi Hauke,

I should have my review wrapped up in the next day or two. Thanks for your patience.

best,

-Shawn


From: Hauke Schulz @.***> Sent: May 31, 2023 10:04:26 AM To: openjournals/joss-reviews Cc: Shawn Chartrand; Mention Subject: Re: [openjournals/joss-reviews] [REVIEW]: PyFlowline a mesh independent river network generator for hydrologic models (Issue #5446)

Hi everyone, I'm glad to see that the review process is in full swing now and the first issues are being created. @andres-patrignanihttps://github.com/andres-patrignani if you could run editorialbot generate my checklist as well that would be awesome. It makes it easier for me to track the process.

— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5446#issuecomment-1570570186, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE7I77S6R5TLYFP5OFQ6Y6TXI5YOPANCNFSM6AAAAAAXXBP3F4. You are receiving this because you were mentioned.Message ID: @.***>

smchartrand commented 1 year ago

I want to start by congratulating @changliao1025 (and Matt Cooper) for their submission. The software authors have tackled a long standing problem in hydrologic modelling, and the outcome is impressive. Thanks for the opportunity to review your submission, and get to explore the code in more detail. I hope my comments are helpful.

Most of my comments below address either (a) challenges with using the .ipynb and .py codes provided in the repository for the Susquehanna example to generate results, or (b) setting the context of the code with respect to existing platforms. All the comments are relatively minor, or moderate in scope, and should be easily addressed. I organized my comments under the review checklist headers to simplify things.

General Checks:

  1. Repository: good.
  2. License: I opened an issue in the home repository regarding the License.
  3. Contribution and authorship, scholarly effort and data sharing: good.
  4. Reproducibility - The primary challenge I encountered was using the .ipynb file [in the notebooks directory] to generate results with pyflowline. The challenges are summarized in the following.
  1. Human and animal research is n/a.

Functionality:

  1. Installation: good.
  2. Functionality - I was able to follow the Quickstart and the Installation instructions separately to successfully install pyflowline. The "Functionality" needs some attention based on my comments above regarding use of the provided Jupyter Notebook mpas_example.ipynb [note: in the Quickstart the notebook is referred to by mpas_notebook.ipynb].
  3. Performance: good - I found no performance issues and the example calculations for the Susquehanna ran in approximately 65 seconds on my laptop [ThinkPad X1 Carbon Intel i7 with 32 GB ram].

Documentation:

  1. Statement of need: good.
  2. Installation instructions are good, but a clear list of dependencies is missing. There are three sources of dependencies and all three provide a different set: requirements_dev.txt [root directory], setup.py [root directory], and requirements.txt [docs directory]. It may make sense to have a requirements.txt file in the root directory that can be linked to installation so the trace is more clear OR clearly state the dependencies in the README on the git home page? The key is to have the dependencies clearly highlighted in one location.
  3. Example usage: good - Authors provide a real-wrold example for the Susquehanna River.
  4. Fucntionality documentation: good.
  5. Automated tests are lacking, but use of the provided python script and Jupyter Notebook with the example offers a manual test of the software.
  6. Community guidelines are clearly addressed in the License.

Software Paper:

  1. Summary - A clear statement of how the software benefits non-specialist audiences would be helpful.
  2. Statement of need - More clear statement of who the intended or target audience is, would be helfpul. In the relating the current software to other and existing tools, the authors discuss just a few examples. I understand the contribution is a first of its kind, but it might be useful to expand this discussion a bit more. Is their merit in mentioning River Network Toolkit (http://rivtoolkit.com/)? Or RivGraph (https://joss.theoj.org/papers/10.21105/joss.02952)?
  3. State of the field - Please elaborate a bit more on what existing packages can do, and why this is not enough (i.e. how pyflowline fills this gap). See comments above under Statement of Need.
  4. References - See comment above under Statement of Need.

filter_flowline

observingClouds commented 1 year ago

Thank you very much for your review @smchartrand. @changliao1025 please feel free to already respond to the issues and comments, while @andres-patrignani is doing their review.

@andres-patrignani could you please create your checklist with @editorialbot generate my checklist? That would be awesome.

changliao1025 commented 1 year ago

@smchartrand Thank you for your comments, which will undoubtedly help us to provide better software. We will address your concerns soon about some details in the document and example code.

Below are a few responses to your concerns:

We will address the remaining comments in the coming weeks. Thank you.

changliao1025 commented 1 year ago

Hi, @smchartrand , Thank you for taking the time to write a review of our work. I appreciate your feedback. Below is the full response to your comments. Thank you for reviewing our submission and recognizing our modeling tool. PyFlowline was developed to close several gaps in hydrologic modeling. For example, although it is designed to run at regional and global scales using any mesh type, many modelers may be more interested in the watershed scale using structured meshes. To this extent, most existing applications can be viewed as a special case in PyFlowline, and PyFlowline provides an opportunity to expand existing applications to other domains and meshes.

General Checks reply:

  1. First, we revised the visualization class and function completely. Because PyFlowline is a core component in HexWatershed and both models share similar visualization patterns (point, polyline, polygon, and mixed), we merged all the visualization feature into an external module within PyFlowline. This module is largely based on another Python package PyEarth, developed by @chango1025. We plan to add PyEarth as a dependency package.

  2. Although visualization is an important component in PyFlowline, it is not a required step to run the model. Besides, our choice of using GeoJSON as the main data I/O format is to ease the visualization task as many tools can visualize GeoJSON. Given the complexity of structured/unstructured datasets that contain point/polyline/polygon, we list the visualization feature as experimental, so users should expect undesired behavior.

  3. We added Geopandas as another option in the notebook so users can easily switch to Geopandas for a quick fix.

  4. Indeed, it is highly possible that the cartopy installation may affect the visualization. As discussed above, all the vector objects in PyFlowline are plotted using a geodetic framework. If some of the spatial reference information is missing, the vector objects may not be plotted correctly.

Functionality reply:

Documentation reply:

Software Paper reply:

Existing river network representation methods often fall into these three categories:

  1. Vector-based, hydrologic models that use this method cannot couple river and land because there is no one-to-one mapping [@Schwenk:2021];
  2. High-resolution DEM-based, only supports structured rectangle grids (e.g., 30m x 30m ) at high spatial resolutions [@Esri:2011];
  3. Upscaling-based, only supports structured geographic grids (e.g., 0.5 degree x 0.5 degree) at coarse resolutions [@Wu:2012]. This method often cannot provide global coverage, including Greenland and the Antarctic.

PyFlowline is the only modeling software that provides these unique features:

  1. It can generate river networks on unstructured meshes;
  2. It uses topological relationships to capture river networks precisely;
  3. It can be applied at both high and coarse resolutions;
  4. It can provide global coverage, including Greenland and the Antarctic.

Thank you.

andres-patrignani commented 1 year ago

Review checklist for @andres-patrignani

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

andres-patrignani commented 1 year ago

General checks

  1. Repository: Good
  2. License: License is available and easy to find.
  3. Contribution and authorship: Author has made substantial contribution to the project repository. Paper authors re appropriate and complete.

Functionality

  1. Installation: Does installation proceed as outlined in the documentation? Yes, but it may be good to indicate that in some cases creating a new conda environment is necessary. It seems to me that most users having already a working conda environment will not attempt to do this.
  2. Functionality: Good
  3. Performance: I did not have any performance issue.

Documentation

  1. Overall the documentation is extensive with tables, figures, an “Overview” section stating the need for the package, and additional detailed information about how to use and get started with the package. I do have some comments to improve the language, which sometimes can lead to unclear statements that may result confusing to users. Below are some suggestions for improvement:
  1. Installation instructions: Instructions and dependencies are clearly stated. However, I had a hard time trying to install the package and making it work. Installation was resolved by creating a separate conda environment and then installing the package.
conda create --name pyflowline_test 
conda install -c conda-forge pyflowline
  1. Example usage: Authors provided a real-world example, but editing all the file paths for the user’s local installation seems overwhelming for a simple example. It would be nice to have a simpler way to set the paths and run the example from the Jupyter Notebook, since this is probably the first thing that users unfamiliar with the package will try to do to become more familiar with it.

  2. Functionality documentation: Great

  3. Automated tests: Good

  4. Community guidelines: There are guidelines pointing users to the FAQ page, developers contact information, and encouragement to submit a GitHub issue if something did not work. I personally made use of the latter option and the authors responded promptly with clear instructions.

Software paper

  1. Summary: There is a clear description of the high-level functionality of the library. It may be good to provide a definition for “structured” and “unstructured” meshes in layman’s terms for non-technical readers. Perhaps the authors can add a figure illustrating both of these concepts and the perils of using structured rectangular meshes to represent river and streamflow networks. I wonder if the authors can explain some of these concepts using the concepts of vector and raster layers, which I think most people with basic training in geographic information systems will be able to understand and follow. I like the addition of a glossary in the online documentation, and it may be good to add the concepts of structured and unstructured meshes to that section.

  2. A statement of need: The statement of need is clear and well-written.

  3. State of the field: This seems to be the only software that can generate river networks on structured and unstructured meshes (so if I understand this correctly, the package is basically mesh-independent).

  4. Quality of writing: The paper well written. The main website needs some clarification (see my comments above)

  5. References: The list of references is complete.

    • The reference by Engwirda, D., & Liao is missing the year and the title is all in upper case letters. Please, adopt a consistent style across all references. According to Zenodo the citation should be: Engwirda, Darren, & Liao, Chang. (2021, October 9). 'Unified' Laguerre-Power Meshes for Coupled Earth System Modelling. 29th International Meshing Roundtable (IMR), Virtual Conference. https://doi.org/10.5281/zenodo.5558988
    • The reference by Liao et al. is missing the year. It seems that this article was published in 2023 according go the DOI.
observingClouds commented 1 year ago

Thank you @andres-patrignani for your review. We really appreciate it.

@changliao1025 I see that you have addressed already most, if not all, of @smchartrand comments. That's great! Could you please go now through @andres-patrignani review and let us know when you addressed all comments? We should then be relatively quick in moving forward.

Cheers!

changliao1025 commented 1 year ago

Hi, @andres-patrignani , Thank you for taking the time to write a review of our work. I appreciate your feedback. Below is the full response to your comments.

General checks reply

Functionality reply

Documentation reply

For example, existing stream-burning methods always treat the vector river networks as a binary mask and cannot describe the topology near river confluences and meanders.

Installing and running PyFlowline requires some basic knowledge of the Python ecosystem. Besides, configuring a PyFlowline simulation requires some knowledge of Geographic Information System (GIS) and computational hydrology.

Software paper reply

In PyFlowline, we define structured meshes (e.g., lat-lon, raster files with projections, and hexagon) as those with fixed cell sizes and shapes and unstructured meshes as those with variable cell sizes and shapes.

We also add them to the glossary.

changliao1025 commented 1 year ago

I want to add that we recently dropped the 'shapely' dependency after we found alternative GDAL APIs.

changliao1025 commented 1 year ago

Merged additional edit from coauthor @mgcooper.

observingClouds commented 1 year ago

Thank you for your update @changliao1025. I will have a look at this later today or tomorrow and will make sure all comments are addressed. Potentially I'll ask the reviewers to confirm that the changes are to their satisfaction.

observingClouds commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

observingClouds commented 1 year ago

Dear @changliao1025, I went through the reviews and responses and found that a few issues remain, some of which seem already addressed but not closed:

might not yet be addressed.

Please close those issues that have already been addressed.

The execution of the example notebook was most challenging for the reviewers especially because some results could only be reproduced by writing additional code, e.g. by using geopandas to plot and evaluate the results. Because the plotting is a substantial part of the evaluation of this software package it is crucial to make this functionality work out of the box (e.g. by making data paths relative to the notebooks location). I would therefore like you and your co-authors to address these outstanding comments, before I kindly ask @smchartrand and @andres-patrignani to check if all the comments have been addressed appropriately and especially check if the example notebook is easy to follow.

Thank you.

changliao1025 commented 1 year ago

@observingClouds We have closed the mentioned issues as they are resolved. Meanwhile, we are improving the file I/O part as it does create some confusion right now.

observingClouds commented 1 year ago

Thanks @changliao1025 for the update. Please let us know when you have improved the I/O part.

changliao1025 commented 11 months ago

Hi,  @observingClouds , sorry for the late updates. We recently made many upgrades to the pyflowline model, including the spatial index algorithm, to speed up the algorithm.

As for the I/O part, we added two additional functions (see example in the notebook), to set and change the model parameters. These improvements are designed to ease how users interact with the model parameters. With these two functions, the users can change the parameters without directly editing the JSON files.

Besides, we simplify the visualization part using GeoPandas. The notebook is only intended to serve as an illustration of the workflow. Due to the computational nature of the model, it is recommended to use the model in the Python environment.

observingClouds commented 11 months ago

Hi @changliao1025,

Thank you very much for the update.

Cheers, Hauke

observingClouds commented 11 months ago

Dear @smchartrand, @andres-patrignani,

Could you please have a last look at this submission and check if your comments have been sufficiently addressed? In response to both of your earlier comments @changliao1025 and his colleagues have now improved the I/O interface. Please have a particular look at this and confirm if everything is to your satisfaction.

Thank you! Hauke

smchartrand commented 11 months ago

Hi All,

I will fit this into my schedule next week.

thanks,

-Shawn


From: Hauke Schulz @.***> Sent: September 30, 2023 9:59:57 PM To: openjournals/joss-reviews Cc: Shawn Chartrand; Mention Subject: Re: [openjournals/joss-reviews] [REVIEW]: PyFlowline a mesh independent river network generator for hydrologic models (Issue #5446)

Dear @smchartrandhttps://github.com/smchartrand, @andres-patrignanihttps://github.com/andres-patrignani,

Could you please have a last look at this submission and check if your comments have been sufficiently addressed? In response to both of your earlier comments @changliao1025https://github.com/changliao1025 and his colleagues have now improved the I/O interface. Please have a particular look at this and confirm if everything is to your satisfaction.

Thank you! Hauke

— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5446#issuecomment-1741955688, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE7I77QUDLALE44DOQP2CSLX5DXZXANCNFSM6AAAAAAXXBP3F4. You are receiving this because you were mentioned.Message ID: @.***>

andres-patrignani commented 10 months ago

I want to thank the authors for the extensive work to improve the pyflowline package. I was able to successfully:

Features that I would like to see implemented in this or future versions (I leave this up to the editor and other reviewers):

Overall I think that the code and the manuscript are in good shape and can be accepted.

Some additional comments for future improvement beyond JOSS:

I hope that my comments have been useful and I commend the authors for creating this library.

Andres

changliao1025 commented 10 months ago

@andres-patrignani Thank you for the thoughtful comments, which helped us a lot in improving our package.

Here are some quick responses to your questions:

Indeed, the mesh generation process, especially unstructured meshes such as the MPAS mesh, has various technical challenges. Currently, pyflowline has built-in support to generate structured meshes, see reference in JAMES paper. But it does not have built-in unstructured mesh generators. Instead, it can read the outputs from these unstructured mesh generators, such as JIGSAW-MPAS by @dengwirda and the newly added DGGRID mesh feature by @sahrk. There are a certain amount of learning curves to using an unstructured mesh generator, considering whether it has good documentation. The MPAS mesh is unique because it involves a range of operations. However, it has many advantages that other meshes cannot provide. That is why we use it as an example to demonstrate the capability of pyflowline to support fully unstructured meshes. In the future, we may add a more straightforward example/notebook to use a different mesh, such as the DGGRID mesh. We have just released such a dataset on Zenodo which can be customized for this purpose.

Again, thanks for the suggestions, and we will keep improving it in future versions.

smchartrand commented 10 months ago

I want to echo the comments by @andres-patrignani, and thank the authors for their extensive efforts to improve pyflowline. All of my previous comments have been addressed (and done so very well), and yesterday I was able to:

I have no further suggestions. Congratulations to the authors on an excellent package. Thanks for the opportunity to review.

observingClouds commented 10 months ago

@andres-patrignani, @smchartrand thank you very much for your time and efforts to review this submission. I appreciate it!

@changliao1025 as you can see from the reviewers' comments all issues have been fixed to their satisfaction. The suggested additional improvements regarding a future release of the software are not necessary to implement for the acceptance in JOSS but I still encourage you to do them in the near future as those might widen your user base.

observingClouds commented 10 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

observingClouds commented 10 months ago

@changliao1025 could you please complete the above mentioned author tasks and ping me when completed? Thank you.

changliao1025 commented 10 months ago

@observingClouds Thank you for the updates. We have checked the items as instructed.

observingClouds commented 10 months ago

@editorialbot set 10.5281/zenodo.6407298 as archive

editorialbot commented 10 months ago

Done! archive is now 10.5281/zenodo.6407298

observingClouds commented 10 months ago

@editorialbot set 0.3.2 as version

editorialbot commented 10 months ago

Done! version is now 0.3.2

observingClouds commented 10 months ago

@editorialbot generate pdf

editorialbot commented 10 months ago

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

observingClouds commented 10 months ago

@changliao1025 thank you for finishing the post-review tasks. I have a few minor comments:

Please address these points and we should be soon ready for publication.

changliao1025 commented 10 months ago

Hi, @observingClouds; all the above issues are fixed now. The version is also bumped to 0.3.4. Thank you for identifying these issues.

observingClouds commented 10 months ago

Thanks @changliao1025. Thanks for changing also the title in the zenodo archive. The newest release is not corrected though (https://zenodo.org/records/10076553).

changliao1025 commented 10 months ago

@observingClouds. I realized that Zenodo only updates the title for a specific version, not all versions. I updated the title for the v0.3.4. Thank you. I may find a way to update the title for all versions.

observingClouds commented 10 months ago

@editorialbot set v0.3.4 as version

editorialbot commented 10 months ago

Done! version is now v0.3.4

observingClouds commented 10 months ago

@editorialbot set 10.5281/zenodo.10076553 as archive

editorialbot commented 10 months ago

Done! archive is now 10.5281/zenodo.10076553