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

270 change check raster grids to take in raster profiles instead of raster data #281

Merged

Conversation

msorvoja
Copy link
Collaborator

Function check_raster_grids in eis_toolkit/utilities/checks/raster.py takes in a sequence of raster profiles instead of raster data now. Tests and some dependent functions were modified, see list edited modules below.

Edited modules:

eis_toolkit/utilities/checks/raster.py
tests/checks/check_raster_grids_test.py
eis_toolkit/raster_processing/unique_combinations.py
eis_toolkit/utilities/checks/crs.py
eis_toolkit/cli.py

Deleted modules (check_raster_grids.py was a duplicate):

docs/raster_processing/check_raster_grids.md
eis_toolkit/raster_processing/check_raster_grids.py

@msorvoja msorvoja added the modification Change to an existing feature label Jan 11, 2024
@msorvoja msorvoja self-assigned this Jan 11, 2024
@msorvoja msorvoja linked an issue Jan 11, 2024 that may be closed by this pull request
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 few small things, otherwise looking good!

eis_toolkit/cli.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep the original implementation unless it didn't work. Raster profile might be missing a CRS in some cases, and this implementation will result in error in that case.

We could also move this test into the raster.py file under checks, as it's only used for rasters (and delete this crs.py file).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nmaarnio Since raster profile (or metadata) is a dictionary, the dot notation won't work (object.crs).

Copy link
Collaborator Author

@msorvoja msorvoja Jan 12, 2024

Choose a reason for hiding this comment

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

How about this?
`@beartype
def check_matching_crs(objects: Iterable) -> bool:
"""Check if every object in a list has a CRS, and that they match.

Args:
    objects: A list of objects to check.

Returns:
    True if everything matches, False if not.
"""
epsg_list = []

for object in objects:
    if not isinstance(object, (rasterio.profiles.Profile, dict)):
        if not object.crs:
            return False
        epsg = object.crs.to_epsg()
        epsg_list.append(epsg)
    else:
        if "crs" in object:
            epsg_list.append(object["crs"])
        else:
            return False

if len(set(epsg_list)) != 1:
    return False

return True`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now it returns False if object is a profile or dict and it does not contain key "crs".

eis_toolkit/utilities/checks/raster.py Outdated Show resolved Hide resolved
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.

Looks good now!

@nmaarnio nmaarnio merged commit 7d18100 into master Jan 12, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modification Change to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change check_raster_grids to inspect only profiles/metadata
2 participants