[REVIEW]: The Region Growing Plugin (RegionGrow)

whedon commented 3 years ago

Submitting author: @gro5-AberUni (Gregory Oakes) Repository: https://github.com/gro5-AberUni/RegionGrow Version: V4.1 Editor: @hugoledoux Reviewer: @jorgegil, @Geogoeroe Archive: Pending

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

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

@jorgegil & @Geogoeroe, 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 @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 @jorgegil

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 (@gro5-AberUni) 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)?
[ ] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[ ] 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 @Geogoeroe

Conflict of interest

[ ] 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

[ ] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[ ] Repository: Is the source code for this software available at the repository url?
[ ] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[ ] Contribution and authorship: Has the submitting author (@gro5-AberUni) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[ ] 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

[ ] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[ ] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[ ] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[ ] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[ ] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[ ] 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

[ ] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[ ] 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?
[ ] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[ ] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[ ] 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?

whedon commented 3 years ago

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

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=1.09 s (30.3 files/s, 82089.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          14            739            519          85775
Qt                               1              0              0           1105
make                             2             62             61            251
Markdown                         4             63              0            190
DOS Batch                        1             21              1            133
TeX                              2              7              0            105
Bourne Shell                     3             14             11             71
HTML                             1              2              0             40
XML                              2              2              0             29
QML                              1              0              0             26
Qt Linguist                      1              0              0             11
reStructuredText                 1              6              6              8
-------------------------------------------------------------------------------
SUM:                            33            916            598          87744
-------------------------------------------------------------------------------

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

Author                     Commits    Insertions      Deletions    % of changes
gro5-AberUni                    24         39656           6400          100.00

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
gro5-AberUni              87033          219.5          5.1                0.39

whedon commented 3 years ago

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

OK DOIs

- 10.5281/ZENODO.4587408 is OK

MISSING DOIs

- 10.9790/2834-0122445 may be a valid DOI for title: Color image segmentation for medical images using L* a* b* color space
- 10.1109/icamechs.2014.6911663 may be a valid DOI for title: Image segmentation algorithm for disease detection of wheat leaves
- 10.1007/978-3-030-05318-5_8 may be a valid DOI for title: TPOT: A tree-based pipeline optimization tool for automating machine learning

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:

Geogoeroe commented 3 years ago

Hi @gro5-AberUni, I got stuck before actually really trying the plugin.... I used the description on downloading, but... The plugin manager says it doesn’t install:

However, it does… with that __MACOSX as an invalid extra plugin.

So I gave it a go anyway. The plugin loads data (both from a UAV image and from a Sentinel dataset).

When the grow function is used (start) it unerringly comes up with the message to increase the treshold. After trying several values in the range from 0-15000, I considered that the plugin probably wasn't installed in a decent way.

I'm running QGIS 3.18 on a Windows 10 environment. Any help would be appreciated.

Cheers, Erik.

gro5-AberUni commented 3 years ago

Hi @gro5-AberUni, I got stuck before actually really trying the plugin.... I used the description on downloading, but... The plugin manager says it doesn’t install:

However, it does… with that __MACOSX as an invalid extra plugin.

So I gave it a go anyway. The plugin loads data (both from a UAV image and from a Sentinel dataset).

When the grow function is used (start) it unerringly comes up with the message to increase the treshold. After trying several values in the range from 0-15000, I considered that the plugin probably wasn't installed in a decent way.

I'm running QGIS 3.18 on a Windows 10 environment. Any help would be appreciated.

Cheers, Erik.

Hi Erik, Thanks for getting in touch.

The first error you received: "No named 'MACOSX/regiongrow'" shouldn't cause any issues, the ZIP directory that QGIS uses to install the plugin was created on a MACOS system which automatically includes a directory 'MACOSX' containing data and resource forks for the zip directory which QGIS was attempting to install as a plugin. I've adjusted the Zip directory within the GitHub repo to not include that directory so you shouldn't see that error.

The Second issue you have noted I believe was caused by a bug where if you digitise a feature close to the left edge of the image, where the neighbourhood would extend beyond the boundaries of the image the plugin would use an incorrect raster origin value and could not digitise the features correctly. I have addressed this issue and tested the plugin on a Windows 10 Machine running QGIS 3.18.3 as well as a MacOS and linux environment and it seems to work correctly.

Please let me know if you come across any further issues Thanks Greg

jorgegil commented 3 years ago

@gro5-AberUni Would it be possible to share the different image files that you use in the paper and documentation?

That would allow reproducing exactly what is presented. It can be cropped images just of the sections shown in the paper. And these could be included in the repository.

Thanks Jorge

gro5-AberUni commented 3 years ago

@gro5-AberUni Would it be possible to share the different image files that you use in the paper and documentation?

That would allow reproducing exactly what is presented. It can be cropped images just of the sections shown in the paper. And these could be included in the repository.

Thanks Jorge

Hi @jorgegil, I have uploaded some example UAV imagery that was used in the documentation and the figures in the paper to the location: gro5-AberUni/RegionGrow/Example_UAV_Data/ so you should be able to reproduce the results we present.

Thanks Greg

Geogoeroe commented 3 years ago

Ah, thanks for the test data. However, on de-installing and re-installing the new version of the plugin, I got this:

Another thing is, that if I deactivate the plugin, it doesn't clear the menu-item on the plugin menu. So after activiating the plugin again (three times), I get this:

gro5-AberUni commented 3 years ago

Ah, thanks for the test data. However, on de-installing and re-installing the new version of the plugin, I got this:

Another thing is, that if I deactivate the plugin, it doesn't clear the menu-item on the plugin menu. So after activiating the plugin again (three times), I get this:

Hi @Geogoeroe It seems as though when I uploaded the UAV data to Github it didn't load the image coordinate system metadata correctly, I've re-uploaded that data to the same location so it should now have the correct metadata attached to the image and that should solve the first issue you highlighted.

The second issue you highlight occurs as a result of QGIS not updating the plugins menu until you restart the application, the next time you load QGIS it should only display the items in the plugin menu once.

Geogoeroe commented 3 years ago

Ah, indeed, that was the issue with the data... I didn't interpret the python error as something data-related, but that took care of it. However, I still have to check the manual properly (I might be doing something wrong here), as the default settings still come up with the treshold warning: 2021-06-21T11:36:03 WARNING Region Grower Plugin : Not Enough Spectral Difference Found. Increase the Threshold 2021-06-21T11:36:03 SUCCESS Region Grower Plugin : Process Successful

Geogoeroe commented 3 years ago

For the updating of the plugins: Yes, I noticed that the menu cleans after restarting QGIS, it is not a permanent thing. However, other plugins don't show this specific behaviour... So it might be a simple trick somewhere.

whedon commented 3 years ago

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

whedon commented 3 years ago

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

jorgegil commented 3 years ago

For the updating of the plugins: Yes, I noticed that the menu cleans after restarting QGIS, it is not a permanent thing. However, other plugins don't show this specific behaviour... So it might be a simple trick somewhere.

Indeed, @gro5-AberUni you must add and remove the plugin from the menu and the toolbar when the plugin is loaded / unloaded. This happens in the main class.

It's rare behaviour (people do not check/uncheck a plugin multiple times) but it has a dev solution and other plugins do it correctly.

jorgegil commented 3 years ago

I've completed a review of the software and the paper and have a long list of comments for both. I'll add them here as separate comments, grouped where relevant, to allow response.

General comment on the paper for now:

The paper is well written but in my view lacks technical detail to make the algorithm clearer and more transparent (details to follow).
A more general use case for the tool must be explained, or indicated that this is meant for the specific use only described in projects.
It lacks a comparison with an existing "Magic Wand" plugin for QGIS.
The images need better captions and be referenced in the text, otherwise they fall out of place and don't help. The paper lacks references.

jorgegil commented 3 years ago

Minor tweaks to the paper:

paragraph 1: "at a continental or global scape" should be scale
paragraph 2: "RegionGrow generates connects pixels with a similar colour" generates should be dropped

jorgegil commented 3 years ago

Paper figures:

figure 1 seems out of place. Either add descriptions to the caption (a-f) as in later figures or refer to it in the text. The steps in "Using this software" have no relation to the steps in figure 1
in fact, you need to refer to all images in the text so we look at them in context and they support the descriptions

jorgegil commented 3 years ago

Use case for the tool:

the section heading "ongoing projects" is not immediately obvious what it refers to: software projects, or applications of the tool.
Maybe it should be a section describing scope, uses cases, and limitations, to explain how it is meant to be used in practice, but also in research. What what it cannot do.
For a tool called region grow I was expecting something like a magic wand in Photoshop: threshold is colour tolerance only, there's no neighbourhood size, and the user can choose contiguous regions or multiple regions. There is a plugin for QGIS called.. "Magic Wand". How is this one different or better/more appropriate for specific tasks?

jorgegil commented 3 years ago

Questions on Step 2 in the paper:

Why does the user need to indicate a distance? Why 25 as a default?
Text should mention "neighbourhood size (m)" which is the label in the tool. The description must be consistent with the text in the GUI.
Why a square neighbourhood, not circle or hexagon?

jorgegil commented 3 years ago

Questions on Step 3 in the paper:

What kind of 'spatial weighting' is it? linear? How is it calculated, in metres?
Shouldn't the spatial relation be based on contiguity rather than distance?

jorgegil commented 3 years ago

Questions on Step 4 in the paper:

At this stage an equation with all aspects of steps 3 and 4, even if simple, would be valuable to make the algorithm very transparent.
Are the values used scaled prior to the sum? What is their range?
What name do you give the 'combined value'?
Needs an explanation for the user defined threshold. How are users supposed to know the right value? Are there reference values (from theory, empirical) or is it trial and error (to get the feel for it)? Why is 15 the default value?
This has an impact on the workflow and its reliability/reproducibility. I see there's more information in the technical guide, this should be included in the paper.

jorgegil commented 3 years ago

Questions on Step 5 in the paper:

Why give option for buffer? It should be explained why the option is there, and example of when/how to use.
Why only geoJSON or shapefile output? Could also create a memory layer like many other plugins and let the user choose how to save it.

jorgegil commented 3 years ago

Testing and validation questions:

Was there any testing and validation done to see if the tool selects what is intended? Should there be a test data set with correct regions to understand thresholds?
The above is necessary, unless the results are not meant to be precise and only rough indications of regions, which applies to a specific use case. In that sense the tool is not a general region identification tool.
Could two rather different colour values that are very close spatially be added to a region, while similar distant ones and still contiguous not? If in the use case for the tool this is not critical, it should be indicated explicitly so that the tool is not misused in other cases.

jorgegil commented 3 years ago

Tool GUI clarity:

Have a starting state of "image type" radio buttons, strange to see all unchecked, not the normal state. Having a default, should it not be Drone?
Image type could be a drop down menu, with Drone imagery on top (the intended use).
What is "Band combinations" for? I don't see explanation in the paper, just a brief mention in the technical guide.
"Add Buffer to regions" should be a checkbox and not a radio button
"Add Buffer to regions" and "feature class value" fields should be before output, or are they meant to be changed while editing?

jorgegil commented 3 years ago

Tool output:

Difference between output file and existing vector data set not explained.
Having two separate fields can be confusing. Is one optional? Does one override the other? There could be a dependency, one field becoming active/inactive depending on the other.
In the layers panel, the added vector layer should have the name given in the "Output vector filename" box, currently has no name.
Existing vector layer could give the option to choose from an existing vector data set from the layers panel?
vector output: why saved with the imagery and no option to choose another location?
vector output results in a memory layer: would give flexibility to save in other formats, or postgresql

jorgegil commented 3 years ago

Tool usability:

Start button should not be active if basic conditions are not met, like having an output file name, or image type.
Ideally there should be a toolbar for the red cross circle button, the class and the undo button. Going to the dialogue box next to start and finish for those actions is not intuitive

hugoledoux commented 3 years ago

@gro5-AberUni the idea is that you consider @jorgegil comments and improve the plugin and let us know here what was modified and how.

👋 @Geogoeroe you haven't ticked any of the chechboxes above, is it because you don't have write access? If so I can reinvite you.

gro5-AberUni commented 3 years ago

@jorgegil @hugoledoux Thank you for the comments about the paper and the plugin itself, I will make the changes and modifications you suggest and let you know when I have completed them.

gro5-AberUni commented 3 years ago

@jorgegil @hugoledoux

Thank you @jorgegil for you comments and suggestions about the plugin and the paper. I have implemented a series of changes to the plugin specifically within the GUI to improve the user performance, notably I have have added a tool bar containing the main buttons for the operation of the plugin in, as well as the option for the user to adjust the feature class value or buffer distance if desired. I have also adjusted the Main Dialogue box to only use one box to specify the output vector dataset.

I have addressed some of the comments that you have made regarding specific aspects of the paper and the operation of the algorithm in general below, but it has not been possible to include or address all aspects within the paper itself due to the word limitations on the paper.

Questions on Step 2 in the paper:

Why does the user need to indicate a distance? Why 25 as a default? As the plugin is designed to highlight a discrete feature around the user’s click, the distance outlines how far around this point the plugin should search for similar pixels which are likely to belong to that feature. As only one feature is to be digitised per click it is not necessary to examine every pixel within the image to determine if it is similar. A distance of 25m metres was selected as it allows for large features to be disgusted in one click, while balancing computational efficiency. Text should mention "neighbourhood size (m)" which is the label in the tool. The description must be consistent with the text in the GUI. Adjusted in the paper to include Neighbourhood Size (m) Why a square neighbourhood, not circle or hexagon? A square neighbourhood was used as it is simpler to implement than a circle or a hexagon neighbourhood and from initial testing a square neighbourhood provides adequate outputs.

Questions on Step 3 in the paper:

What kind of 'spatial weighting' is it? linear? How is it calculated, in metres? Spatial weighting is linear based on euclidean distance from the centre of the generated neighbourhood outwards

Questions on Step 4 in the paper:

At this stage an equation with all aspects of steps 3 and 4, even if simple, would be valuable to make the algorithm very transparent. An Equation has been added to the paper in step 4 to indicate how colour distance and the spatial weighting is applied to select candidate pixels. Are the values used scaled prior to the sum? What is their range? From development and testing it was decided that values dont need to be scaled to be within a specific range What name do you give the 'combined value'? In the algorithm it is referred to as totalWeightedDistance but this definition has little relevance to the user. Needs an explanation for the user defined threshold. How are users supposed to know the right value? Are there reference values (from theory, empirical) or is it trial and error (to get the feel for it)? Why is 15 the default value? Default values were selected for image types during testing and development and are provided as a guide, in the user guide it will be included that the user can change this based on the general feel of how the tool is performing. The undo feature can be helpful to allow a user to tune the digitised feature. This has an impact on the workflow and its reliability/reproducibility. I see there's more information in the technical guide, this should be included in the paper.

Questions on Step 5 in the paper:

Why give option for buffer? It should be explained why the option is there, and example of when/how to use. The option for a buffer on the digitised feature is included to allow the user to select a larger region for example if they are using the tool to highlight an area to direct to field workers. This would be inappropriate if the user was using the tool to generate training data for a supervised image classification as spurious pixels would be included. Why only geoJSON or shapefile output? Could also create a memory layer like many other plugins and let the user choose how to save it. GeoJson and and Shapefile formats are used as they are the common formats that have been used within the Spatial Intelligence System for Precision Larviciding where the plugin was initially developed. It is possible to export the digitised layer after processing has finished by exporting the layer in the desired format from the layers panel.

Testing and validation questions:

Was there any testing and validation done to see if the tool selects what is intended? Should there be a test data set with correct regions to understand thresholds? Testing was performed to ensure that he plugin was digitising features suitably but it was decided that no formal validation routine was particularly necessary particularly with user defined variables. The above is necessary, unless the results are not meant to be precise and only rough indications of regions, which applies to a specific use case. In that sense the tool is not a general region identification tool. The results are not designed to a definitive mask layer rather more of an indication, as is the case with other region growing tools such as that sound in the Semi Automatic classification plugin or MagicWand. Could two rather different colour values that are very close spatially be added to a region, while similar distant ones and still contiguous not? If in the use case for the tool this is not critical, it should be indicated explicitly so that the tool is not misused in other cases. It would be unlikely to find pixels with very different colours in the same digitised features as when plotted in the 3D colour feature space these pixels will have a very large colour distance which would be greater than the influence of their spatial proximity. Continuous pixels of similar colour are more likely to be digitised as the same feature as the colour distance within the 3D feature space has a more influence than the spatial weighting This will be outlined in the paper.

Tool GUI clarity:

Have a starting state of "image type" radio buttons, strange to see all unchecked, not the normal state. Having a default, should it not be Drone? GUI adjusted to have the drone option as default. Image type could be a drop down menu, with Drone imagery on top (the intended use). What is "Band combinations" for? I don't see explanation in the paper, just a brief mention in the technical guide. Band Combination is used primarily when dealing with multispectral data to allow to use to select a band combination more suited to visualising targets of interest within the imagery. For example with Landsat Imagery the Near Infrared, Red and Green Bands may be used to the digitisation of healthy vegetation. "Add Buffer to regions" should be a checkbox and not a radio button This has been Changed "Add Buffer to regions" and "feature class value" fields should be before output, or are they meant to be changed while editing? They are after the output box as they can be changed on the fly after the plugin is started.

Tool output:

Difference between output file and existing vector data set not explained. This has been removed so thee is only one output vector dataset box Having two separate fields can be confusing. Is one optional? Does one override the other? There could be a dependency, one field becoming active/inactive depending on the other. This has been removed so thee is only one output vector dataset box In the layers panel, the added vector layer should have the name given in the "Output vector filename" box, currently has no name. The vector dataset in the layers menu is now called ‘Digitised Features’ Existing vector layer could give the option to choose from an existing vector data set from the layers panel? This has been removed so thee is only one output vector dataset box vector output: why saved with the imagery and no option to choose another location? This has been removed so thee is only one output vector dataset box vector output results in a memory layer: would give flexibility to save in other formats, or postgresql It was decided to commit all changes to a saved vector file after each feature was digitised to reduce the chance work is lost if there is a computer issue. This was observed when testing the plugin operationally in Zanzibar where high temperatures could cause issues to computers while digitisation was being performed.

Tool usability:

Start button should not be active if basic conditions are not met, like having an output file name, or image type. All conditions must now be met before the start button is activated. Ideally there should be a toolbar for the red cross circle button, the class and the undo button. Going to the dialogue box next to start and finish for those actions is not intuitive A toolbar has been added which includes the Undo, Reactivate Tool, Finish, and change feature class number and buffer distance on the fly should the user need this feature.

jorgegil commented 3 years ago

@gro5-AberUni Thank you for the very comprehensive response to my comments and revision of the paper and plugin.

I'm happy with the changes and recommend the paper to be accepted pending some minor edits.

Under Statement of Need section:

"Within the open source software domain, there are very region growing tools available." instead of "very" maybe "other" or nothing
"which are able to select regions within imagery of similar colour," should read "select regions of similar colour within imagery"
revise "However if desired features are outside of this set distance they will not be included in the vector dataset and a new vector layer will have to be produced to include these layers." Maybe last word should be features?

Under step 4.

The equation does not need a caption, only a number on the right of it.
A text revision is recommended for possible typos and style issues.

In the GUI of the plugin:

Image type: it is still a series of radio buttons without a default checked, when I chose an input image. Maybe you don't want to have the default checked, to make it an explicit choice of the user?
The 'RGB Drone Image' could be the first item in the radio list.
The style of the radio button items should be aligned left, like other widgets in the dialog
Given that there is a toolbar (great addition!) the dialog can disappear once start is clicked and the image appears, to let the user start digitising right away.

gro5-AberUni commented 3 years ago

@jorgegil Thank you for your further comments regarding the paper and the plugin GUI, i have updated the paper and made corrections where appropriate.

Regarding the GUI i have adjusted the radio buttons so that the RGB drone button is selected by default when the image file is selected and adjusted the layout of the radio buttons and label so they are consistent with the rest of the items in the dialogue box. I have also made the change you suggested where the dialogue box is now sent to the background behind the main QGIS window.

Geogoeroe commented 3 years ago

@gro5-AberUni the idea is that you consider @jorgegil comments and improve the plugin and let us know here what was modified and how.

👋 @Geogoeroe you haven't ticked any of the chechboxes above, is it because you don't have write access? If so I can reinvite you.

Hi all, I really hate to say and do this, but... I give up.

Today, refreshed from my holiday, I gave it another shot. Everything looks good about this plugin: simple, clean, manual is clear. But I just can't get it to work. Today I downloaded a fresh version of QGIS (latest), fresh install of the newly downloaded plugin, with the extra menu items in the toolbar and all. Python error. Started with a new profile, different Python error. Went back to my LTR 3.16 with a new profile and the new download, python error. Again.

So basically, I've seen it in action one time, before my holiday, there was one moment when I had it running. But after shutting QGIS down and restarting it the next day, it was all errors again (same thing I've been mentioning before, the warning that I should change my parameter settings because it didn't return any pixels from where I clicked. And now I'm back to Python errors again.

It feels like a kind of personal defeat here, especially because this apparently doesn't happen with you guys. Maybe it's windows that's bugging me? Really coudn't say, but... this was a very frustrating experience.

I'm really happy to try again if it works for me, as I am more into trying functionality of plugins, not into debugging errors. Sorry.

Cheers, Erik.

gro5-AberUni commented 3 years ago

@Geogoeroe Hi Erik, I'm sorry to hear that you have been struggling with the plugin, would it be possible to get a screenshot of the python errors that you have received so I can attempt to debug the issues?

Are you receiving an error message saying: 'Not Enough Spectral Difference Found. Increase the Threshold'? If so this error message is no longer present in the most recent version of the plugin, so if you are seeing this message it could perhaps indicate that there is a plugin version conflict somewhere, when you installed the most recent version of the plugin from github did you uninstall the existing version from within the QGIS 'Manage Plugins' window?

Thanks for your feedback, Greg

Geogoeroe commented 3 years ago

Hi Greg, I'll check the error messages tomorrow and will put some here (there were different ones on different occasions, couldn't get a fix on the actual problem, was really weird). For the other error message about the spectral difference: I didn't get that far today, last one was from last month I'd say. Sorry, wrote a little frustrated I think ;-).

Erik.

Geogoeroe commented 3 years ago

2021-08-13 12_46_50-_Untitled Project — QGIS straight from the box!

This one was the one I could replicate now. Just to be sure, I've really cleaned out my QGIS residual stuff from my computer).

After starting QGIS with a new profile and trying exactly the same, I got this: 2021-08-13 13_04_00-_Naamloos project — QGIS brandschoon

Which is interesting, it looks like it cannot create a new geojson file.

I've made a small video (it's unlisted, but can be reached through this link) to see what's happening here. Maybe I'm really doing something wrong here, the video should show that as well.

hugoledoux commented 3 years ago

thanks @Geogoeroe for your extra efforts, I hope you still like JOSS 😀

What I propose is this: if @gro5-AberUni can figure out what is wrong then we can continue, if not then as the editor I have to go double-check that users using Windows can install and use it. So I'll try to find someone who uses Windows and is knowledgeable in this area.

Geogoeroe commented 3 years ago

Tomorrow I'll meet with a few other QGIS friends, I plan to let them check as well, to see it's not just my setup :-)

Geogoeroe commented 3 years ago

All right, we checked. On 4 different windows laptops the tool is not working. In order to see if it is a windows thing, we also checked on a Linux laptop. Also, not working. Then, after trying a couple of times, it worked, for no apparent reason (on the linux laptop).

My take on this: code is currently too buggy to review properly. Not fit for use yet, sorry to say.

gro5-AberUni commented 3 years ago

@Geogoeroe I'm sorry to hear you have still had issues, I have checked the errors you have received and the video you uploaded. It seems as though the second issue you're seeing is a result of the incorrect use of the save vector file, following feedback from @jorgegil the plugin GUI and documentation has been updated so that the vector dataset is saved using the system file dialogue box to be in line with other QGIS plugins. I will continue to debug the first issue you have received as I have not seen this behaviour when I have tested the plugin on windows, Linux or MacOS machines. There is an example dataset included within the plugin folder which I have tested with the plugin and can be used to test functionality

hugoledoux commented 3 years ago

thanks @Geogoeroe for having the code/plugin tested on other machines.

Unfortunately @gro5-AberUni , to be accepted the code should really run on all 3 major platforms without issues. I propose you indeed try to fix the 2nd issue, and then if you could test it yourself on those platforms that would be best.

The content of the paper and the scope of the work is fine, but we need to have it working.

Let us know when you have fixed the errors.

gro5-AberUni commented 3 years ago

Hi @Geogoeroe

To assist me in debugging the issue would you be able to let me know of some details regarding the Drone Image you are using so i can try to replicate the error that you are receiving. From digging into the code i think the issue may being caused by some unexpected behaviour with the image itself rather than a platform dependent issue.

Could you please tell me what the image coordinate system is, the image extent and the pixel resolution, knowing this information might help me figure out whats going on with the code.

Thanks very much for your help.

Greg

Geogoeroe commented 3 years ago

The image is in 32631 - WGS 84 UTM zone 31N. File is attached.

However, the same thing happened with the supplied test data (I tried this with the project CRS was at 4326, at 3857, and (because I wasn't paying attention) with 28992). odm_orthophoto.zip

By the way, I do know about the use of the save vector file. I never do it this way (you always go through the ... button), but the documentation specifically stated that it would automatically save in the same directory as the data would be. But I tried it with the complete path as well.

My other suggestions would be (I would give these in a normal review, but hey):

you could use the option to load a raster from one of the layers in the project. In general, that makes more sense to end-users than to add another layer (probably one that they ae already viewing).
delete all text next to the buttons in the toolbar. No other plugin has this, in my experience. It also kind of defeats the purpose of the button. You could make a menu entry instead for textual clarification of the tools.
geojson and shapefiles are okay as output options, but why not use a geopackage? Makes more sense nowadays.

Cheers, Erik.

hugoledoux commented 3 years ago

Hello everyone, I'm away on holidays for 3w, back second week of September.

/ooo tomorrow until September 13

gro5-AberUni commented 3 years ago

@hugoledoux

I've continued to test the plugin on various different computer systems running Windows, MacOS and Linux operating systems using different combination of QGIS versions on each, and trying a brand new fresh Windows 10 install with QGIs and i have not be been ale to recreate the errors that haver been seen in the comments above. I have used tested using both the example imagery i provide with the plugin, as well as the data provided above by Erik and each combination of system and QGIS version all seemed to work fine with no issues. I've also asked colleagues at other institutions toi test the plugin on their systems and they've reported no issues either.

gro5-AberUni commented 3 years ago

@hugoledoux

Would it be possible to get an update/advice about how to proceed with the paper submission.

Thanks

Greg

hugoledoux commented 3 years ago

Sorry for the long delay @gro5-AberUni , I had to allocate time to go through this myself, and some of the issues raised by the reviewers raised bigger issues for JOSS editors (what is an acceptable QGIS plugin?).

First my own experience. I am on macOS with QGIS3.16 and I have to say that I ran into the same kind of Python errors that @Geogoeroe reported... I tried to classify the drone image in the Example_UAV_Imagery folder. I can start but then suddenly I do get errors and it's confusing, eg https://cln.sh/hIM5nb and https://cln.sh/YS3fQM (no finish possible?) and https://cln.sh/UthDTh (while adding new classes).

I believe that one source of confusion is that the plugin doesn't follow the standard guidelines of other QGIS plugins. I mean by this, eg:

the GUI is not "QGIS-compliant". For most other plugins, you use one of the layers already loaded for instance, and the resulting dataset is a virtual layer. This virtual layer can then be saved as any of the formats that QGIS supports (instead of just GeoJSON and Shapefile in your case). This means using the classes/functions of QGIS to save a file, and not rely on other Python packages.
the plugin takes over the whole GUI by changing the mouse icon, and even if I fall back on the standard GUI, I still see the icon. Is this a bug, or supposed to be this way? And the icon is the "processing/doing something" icon, so this adds confusion too.
It is not clear whether I should use the plugin interface or the toolbar now. I would say that it should be one of them, not both.
If the plugin is a window, then this window needs to be dockable so that I see it while I'm working. If it's a toolbar, then the starting window could be only to setup the files and start.
some important and very frequent errors, eg if the output file is already present, are only reported in the python console? (https://cln.sh/GPR3Ql) While the start button shouldn't even allow me to start, no? Or a standard dialog telling me it exists and if I want to overwrite it.

I do believe that the plugin your submitted can be useful for others and offers something extra to the other plugins in the QGIS repository, but since the algorithm behind is rather simple it's the GUI that makes the plugin valuable. And unfortunately, in its current state, it is not following the QGIS standards.

So I propose that you let us know if you want to continue this review and make the plugin compliant with the demands above. If yes then we're happy to review again and help. When the fixes are completed, and the plugin in is the official QGIS plugin repository, then the submission would be accepted.

If you believe this is too much work (I am aware it is) and not worth it, then I would understand too.

Let me know.

hugoledoux commented 2 years ago

@gro5-AberUni could you please let us know if you plan to work on the required changes/updates?

hugoledoux commented 2 years ago

I have to conclude that the paper will not be further updated by the author (to fulfill the demands stated above), I have waited more than 1 month for a confirmation now.

We will therefore stop this review, thanks @jorgegil and @Geogoeroe for your contributions.

If you choose in the future to update and improve the plugin then a new submission is possible.

hugoledoux commented 2 years ago

@arfon This review should be closed, but I am not sure what the exact procedure is (and whether I have the power to do this).

openjournals / joss-reviews