Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add region growing algorithm #13

Merged
merged 9 commits into from
Aug 30, 2023

Conversation

oberonia78
Copy link
Contributor

@oberonia78 oberonia78 commented Jun 24, 2023

This PR adds the region-growing algorithm to DSWx-SAR.
The region-growing algorithm takes the fuzzy-value Geotiff and produces the intermediate binary water map, region_growing_output_binary_VV_VH.tif.

region_growing

# preprocessing (relocating ancillary data and filtering)
pre_processing.run(cfg)

# create dummy water map.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# create dummy water map.
# create dummy water map. This will be replaced or removed in future.

data_shape,
pad_shape)
# run region-growing for blocks in parallel
result = Parallel(n_jobs=10)(delayed(process_region_growing_block)(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to be fixed

Suggested change
result = Parallel(n_jobs=10)(delayed(process_region_growing_block)(
result = Parallel(n_jobs=-1)(delayed(process_region_growing_block)(

water_binary[buffer_binary] = new_water
number_added = np.sum(new_water)
itercount = itercount + 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print number of iteration and number of pixels added.

src/dswx_sar/dswx_sar_util.py Outdated Show resolved Hide resolved
src/dswx_sar/dswx_sar_util.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
@oberonia78
Copy link
Contributor Author

@LiangJYu Thank you for the comments. I revised the PR based on your comments.

src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
Parameters
----------
fuzz_image : numpy.ndarray
fuzzy image with values [0, 1] where 0 is non-water and 1 is water
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fuzzy image with values [0, 1] where 0 is non-water and 1 is water
fuzzy image with values [0, 1] representing likelihood of water where 0 is 0% and 1 is 100%

Does the above better describe fuzz_image? If so, would water_likeihood_image be a more specific name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestions. But, region-growing is used in different ways in initial thresholding, regardless of water. So more general name seems better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you like likelihood_image? It's more specific than fuzz but still general since water is not in the variable name. Or is my interpretation of values as [0-1] as a likelihood incorrect or too narrow?

Also for clarification about the comment above, do you mean this region-growing code can/will be used for features besides water? If so, would it make sense remove water from variables names?

Sorry for all these questions...

src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
Comment on lines 30 to 31
initial_seed : float
Threshold used for the seed of water body [0 ~ 1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
initial_seed : float
Threshold used for the seed of water body [0 ~ 1]
initial_water_threshold : float
Initial threshold [0 - 1] used to classify
`fuzz_image` into water and non-water pixels.
If a pixel values exceeds `initial_water_threshold`
then it is classified as water, otherwise it is
classified as non-water.

If I'm understanding the purpose of algorithm, I think the description above may better describe the purpose of this parameter.

For my curiosity, why is the initial threshold higher than tolerance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your description is more specific and better for this specific purpose. But in my opinion, including 'water' in the variable name may be too specific.

The reason why the initial threshold is always higher than tolerance is that the region growing starts from the initial threshold (seed) and classify the fuzzy image with the initial threshold to initially find the high likelihood of water and remove the wrong classified pixels.

After that, in the next loop, the code looks for adjacent pixels within the relaxed values to find more water areas. This is because some pixels in water objects can have slightly lower fuzzy values due to the windy conditions.

Copy link
Contributor

@LiangJYu LiangJYu Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the explanation.

What do you think of either seed_threshold or initial_threshold? Without looking at the docstring I think either suggestion is more specific and there's a naming consistency that implies from the looking at the parameters of the relation to relaxed_threshold.

src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
Comment on lines 1 to 10
import time
import os
import numpy as np
from scipy import ndimage
import cv2

import time
import logging
import mimetypes
from joblib import Parallel, delayed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imports are not ordered according to pep8. I recommend running this through a linter after the PR gets its 2 approvals to resolve.

src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
src/dswx_sar/region_growing.py Outdated Show resolved Hide resolved
@oberonia78
Copy link
Contributor Author

@LiangJYu Thank you for your great comments and corrections. I just finished the modification. Would you be able to have another look?

nb_components, *_ = cv2.connectedComponentsWithStats(
thresh,
connectivity=8)
nb_components = nb_components - 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nb_components = nb_components - 1
nb_components -= 1

nit - concise

Parameters
----------
fuzz_image : numpy.ndarray
fuzzy image with values [0, 1] where 0 is non-water and 1 is water
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you like likelihood_image? It's more specific than fuzz but still general since water is not in the variable name. Or is my interpretation of values as [0-1] as a likelihood incorrect or too narrow?

Also for clarification about the comment above, do you mean this region-growing code can/will be used for features besides water? If so, would it make sense remove water from variables names?

Sorry for all these questions...

Copy link
Contributor

@LiangJYu LiangJYu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits. Sorry for not getting to this sooner.

initial_seed=0.6,
relaxed_threshold=0.45,
maxiter=200):
"""Process region growing using parallel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Process region growing using parallel
"""Perform region growing in parallel

nit - wording

@oberonia78
Copy link
Contributor Author

@LiangJYu Thank you for your suggestions. I just removed 'water' from the variable names. likelihood_image seems reasonable to me. If you don't have additional comments on this PR, may I merge it?

#13 (comment)

Copy link
Contributor

@LiangJYu LiangJYu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits with a couple questions. Should be good to go after this.

Comment on lines 27 to 28
fuzzy image with values [0, 1]representing
likelihood of water where 0 is 0% and 1 is 100%
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fuzzy image with values [0, 1]representing
likelihood of water where 0 is 0% and 1 is 100%
fuzzy image with values [0, 1] representing
likelihood of feature (e.g. water) where 0 is 0% and 1 is 100%

likelihood_image : numpy.ndarray
fuzzy image with values [0, 1]representing
likelihood of water where 0 is 0% and 1 is 100%
initial_seed : float
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of either seed_threshold or initial_threshold to replace initial_seed? Without needing to look at the docstring, I think either suggestion is more specific/descriptive and there's a naming consistency that implies a relation to relaxed_threshold.

Comment on lines 31 to 39
`fuzz_image` into water and non-water pixels.
If a pixel values exceeds `initial_seed`
then it is classified as water, otherwise it is
classified as non-water.
relaxed_threshold : float
relaxed threshold to be used for transient area
between water and non-water. Any pixel value
greater than threshold classified as water,
otherwise it's classified as non-water.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`fuzz_image` into water and non-water pixels.
If a pixel values exceeds `initial_seed`
then it is classified as water, otherwise it is
classified as non-water.
relaxed_threshold : float
relaxed threshold to be used for transient area
between water and non-water. Any pixel value
greater than threshold classified as water,
otherwise it's classified as non-water.
`fuzz_image` into feature and non-feature pixels.
If a pixel values exceeds `initial_seed`
then it is classified as feature, otherwise it is
classified as non-feature.
relaxed_threshold : float
relaxed threshold to be used for transient area
between feature and non-feature. Any pixel value
greater than threshold classified as feature,
otherwise it's classified as non-feature.

More replacements of water with feature.

Comment on lines 108 to 116
`likelihood_image` into water and non-water pixels.
If a pixel values exceeds `initial_seed`
then it is classified as water, otherwise it is
classified as non-water.
relaxed_threshold : float
relaxed threshold to be used for transient area
between water and non-water. Any pixel value
greater than threshold classified as water,
otherwise it's classified as non-water.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace water with feature in docstring.

Comment on lines 168 to 177
`likelihood_image` into water and non-water pixels.
If a pixel values exceeds `initial_seed`
then it is classified as water, otherwise it is
classified as non-water.
relaxed_threshold: float
value where region-growing stops.
relaxed threshold to be used for transient area
between water and non-water. Any pixel value
greater than threshold classified as water,
otherwise it's classified as non-water.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More replacement of water with feature needed in this docstring.

data_block: np.ndarray
Block read from raster with shape specified in block_param.
'''

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

nit - whitespace/extra row

block_param: BlockParam
Object specifying where and how much to write to out_raster.
'''

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

nit -white space/extra row

1: the pixels involved in region growing (i.e., water)
0: the pixels not involved in region growing (i.e., non-water)
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

nit - white space/extra row

data_block: numpy.ndarraya
fuzzy values after region growing
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

white space/extra row

maxiter: integer
maximum number of dilation
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

white space/extra row

@oberonia78 oberonia78 merged commit 594c06a into opera-adt:main Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants