proximity tool to convert the 0's and 1's in the polygon #432
Conversation
…, we get from the distance compuation
|
Hey, a quick review: The tool should not take in an already computed distance array, but instead perform the distance computation within this proximity tool. It is already possible to first run distance computation and then apply whatever transformation/scaling to the output, but now we want convenience (=user runs just one tool to produce the proximity raster). Also, maximum distance parameter should not be optional here, since the output raster does not really make sense if the values scale up to the boundaries (unless the user specifically sets the max distance so large that it reaches the edges). |
|
Also see issue #430 which was made for this feature |
Okay, I get it now. |
|
No need to rename branch or anything, the issue you based this feature on was a little misleading (should have been closed already probably) and can be closed when this PR is merged. Additionally #430 will be closed when this is merged. |
…ance as inputs and performs the inversion. Logarithmic scaling is also introduced as result of previous discussions
|
The tool now takes in a GeoDataFrame, raster profile and maximum distance as inputs and performs the inversion by calling distance computation function inside the tool itself. Logarithmic scaling is also introduced as result of previous.
The logarithmic scaling results are shown below
|
There was a problem hiding this comment.
Hi Omkar, did a full review now. Apologies for the strict format rules of EIS Toolkit, but we try to keep the toolkit harmonized and tidy.
Besides the comments I wrote, please add a .md file in the docs folder and unit tests (see some other unit tests in EIS Toolkit for examples, no need to include many).
Additionally, you could add a CLI function in cli.py for proximity_computation, but I can take care of that after this PR has been merged if you want.
There was a problem hiding this comment.
Please rename this file to proximity_computation.py (and related files accordingly) to follow the naming schema of distance computation.
| from numbers import Number | ||
| from beartype import beartype | ||
| from beartype.typing import Union | ||
| from rasterio import profiles, transform |
There was a problem hiding this comment.
transform is unused import
| from eis_toolkit.vector_processing.distance_computation import distance_computation | ||
|
|
||
| @beartype | ||
| def calculate_proximity(geodataframe: gpd.GeoDataFrame, raster_profile: Union[profiles.Profile, dict], maximum_distance: Number) -> np.ndarray: |
There was a problem hiding this comment.
This line, as some other lines, exceeds the allowed limit. If you use pre-commit locally, it will fix/point out these kind of cosmetic issues.
Also, perhaps it is better if the tool is named similarly as distance_computation is right now, so proximity_computation
There was a problem hiding this comment.
I had an issue with the pre-commit hooke
pip is configured with locations that require TLS/SSL, however the ssl module in Python is not available
However, is now resolved. I ran the precommit hooke and cleared the cosmetic problems
| """ Interpolates the distance values calculated by the distance_computation function between 0 and 1. | ||
| 1 denots the value inside the polygon and 0 at the maximum distance. | ||
| If maximum_distance value is not provided, the program sets this value to the maximum value | ||
| in the provided distance matrix. | ||
| Uses linear interpolation to calculate the distance from the polygon. |
There was a problem hiding this comment.
The format of the docstring should follow that of other tools. I believe the line "If maximum_distance value is not provided, the program sets this value to the maximum value " is also outdated as maximum_distance is required
| return out_matrix | ||
|
|
||
| @beartype | ||
| def calculate_logarithmic_proximity(geodataframe: gpd.GeoDataFrame, raster_profile: Union[profiles.Profile, dict], maximum_distance: Number) -> np.ndarray: |
There was a problem hiding this comment.
I think it would be better if the choice of scaling should be parameterized, not formulated as another public function. So, let's expose only one proximity_computation to the users with parameter scaling. It could be literal Literal['linear', 'log'] with linear as default.
| minimum = np.min(out_matrix) | ||
| difference = maximum_distance - minimum | ||
| out_matrix = maximum_distance - out_matrix | ||
| out_matrix = out_matrix/difference |
There was a problem hiding this comment.
I believe you could utilize _min_max_scaling from EIS Toolkit although I am not 100% sure. Please check and if it fits, we could replace this piece of code with a call to that function to have the operation centralized in one place only.
There's also the point that if we extend the scaling options in the future, it is good to use our own transformations module wherever we can
There was a problem hiding this comment.
I tried using the _min_max_scaling, using this tool I could not achieve the inversion we intend to achieve. Moreover, I cannot see the scaling happening as well. Below I have attached the pictures.
| modified_distance_array = np.where(distance_array==0.0,np.nan,distance_array) | ||
| out_matrix = np.log(modified_distance_array) | ||
|
|
||
| log_maximum = np.log(maximum_distance) | ||
| minimum = np.min(distance_array) | ||
| if(minimum != 0): | ||
| log_minimum = np.log(minimum) | ||
| else : | ||
| log_minimum = minimum | ||
| difference = log_maximum - log_minimum | ||
| out_matrix = log_maximum - out_matrix | ||
| out_matrix = out_matrix/difference | ||
|
|
||
| out_matrix = np.nan_to_num(out_matrix,nan=log_maximum) |
There was a problem hiding this comment.
Check also if the transformations module would be of use to perform the log transform.
As discussed during the meetings, where it was requested to add a new tool that can convert the 0's and 1's in the polygon has been created.
I have added the new proximity tool to invert the 0's and 1's in the polygon we get from the distance computation.
I have just used only linear interpolation as of now.