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 unique grid combinations #86

Merged
merged 23 commits into from
Nov 9, 2023
Merged

Add unique grid combinations #86

merged 23 commits into from
Nov 9, 2023

Conversation

lehtonenp
Copy link
Collaborator

This functionality creates a raster of unique combinations using 2 or more raster bands. The bands can be in different files.
For example, if the user would input two rasters with the following values:
[0,1,0,1,1]
[1,1,1,1,0]
The output combinations would be:
[1,2,1,2,3].
The number 1 would be the first combination, 2 is the second combination, and 3 is the third combination found. While the first and third combinations have the same values but in different rasters, those are still unique combinations.

Copy link
Collaborator

@nmaarnio nmaarnio left a comment

Choose a reason for hiding this comment

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

Hi! I pointed out a few problems as comments. Besides these, I wondered more generally whether this unique_combinations should take rasters as inputs or 2D arrays (basically raster bands). If this functionality is used only for rasters, it makes sense to take rasters as inputs, otherwise I would opt for a more general array function. Secondly, I'm thinking if this function is supposed to take only rasters with same grid size and location (so, unified/co-registered rasters), or is it okay if the rasters do not actually align? Currently this is not tested in any way at least. Maybe @jtorppa or @chudasama-bijal have an opinion on these too?

eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
tests/unique_combinations_test.py Outdated Show resolved Hide resolved
@nmaarnio
Copy link
Collaborator

Looks good to me now! I noticed however that the linter wants to change the single quotation marks to double ones on lines 44, 45 and 46. Also it wants to change the order of some imports and add spaces around "+" on line 49. Did you forget to run the linter, or is there some differences how it behaves in different development environments I wonder? If the latter, I need to investigate this matter soon

@lehtonenp
Copy link
Collaborator Author

I noticed however that the linter wants to change the single quotation marks to double ones on lines 44, 45 and 46. Also it wants to change the order of some imports and add spaces around "+" on line 49...

I ran the linter and got no errors where you pointed out. Could you then tell me the proper order of the imports?

@nmaarnio
Copy link
Collaborator

Okay, good to know. This is the order of imports for me:
import itertools
from typing import Dict, List, Tuple

import numpy as np
import rasterio

from eis_toolkit.exceptions import InvalidParameterValueException

Copy link
Collaborator

@nmaarnio nmaarnio left a comment

Choose a reason for hiding this comment

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

Hi, I took a look at this and wrote a few comments.

eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
tests/raster_processing/unique_combinations_test.py Outdated Show resolved Hide resolved
eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
@lehtonenp
Copy link
Collaborator Author

@nmaarnio I have now used numpy to get the unique combinations. It is faster with a 10k x 10k grid compared to the old one (95 sec to 85 sec). I apologize for the bad naming of the commits. I made commits called 'Use numpy to find unique combinations without nested loops'. Somehow, with my lack of understanding of pre-commit, the commits got lost in the way.

@nmaarnio
Copy link
Collaborator

nmaarnio commented Nov 8, 2023

@lehtonenp is this ready for a new review, or are you still planning to make modifications?

@lehtonenp
Copy link
Collaborator Author

@lehtonenp is this ready for a new review, or are you still planning to make modifications?

@nmaarnio this is now ready for review. The function should work as intended but the combination number is not in running order.

Copy link
Collaborator

@nmaarnio nmaarnio left a comment

Choose a reason for hiding this comment

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

Just a couple nitpicky comments regarding docstring convention and typing, otherwise looks good!

eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
eis_toolkit/raster_processing/unique_combinations.py Outdated Show resolved Hide resolved
@nmaarnio
Copy link
Collaborator

nmaarnio commented Nov 9, 2023

LGTM, merging!

@nmaarnio nmaarnio merged commit 86088f5 into master Nov 9, 2023
4 checks passed
@nmaarnio nmaarnio deleted the add_unique_grid_conditions branch February 19, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants