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

Change default behaviour of crop to inplace=False #439

Merged
merged 10 commits into from
Jan 18, 2024

Conversation

atedstone
Copy link
Member

This PR creates breaking changes to the API

Resolves part of #432: Raster.crop() is now inplace=False by default. This aligns its functionality with Raster.reproject() and with other projects more generally such as xarray.

The PR also fixes a bug in test_projtools:test_utm_to_epsg. A KeyError was being returned by proj_tools.utm_to_epsg():

        with pytest.raises(TypeError):
            pt.utm_to_epsg({"utm": "10N"})  # type: ignore

This is now fixed by requiring a str type in utm_to_epsg(), allowing all tests to pass locally on my machine.

@atedstone atedstone requested a review from rhugonnet January 18, 2024 10:59
@atedstone
Copy link
Member Author

I should also note that I checked for other possible uses of inplace functionality. As far as I see, only crop() has this option, so there are no further functions to change.

@rhugonnet
Copy link
Member

Great @atedstone!
Curious that only these tests broke... I think by leaving the None return for inplace there might be more to change! (in the doc examples in particular)

@atedstone
Copy link
Member Author

I now realise that Vector.crop() also needs changing - will do this and add to this PR.

@atedstone
Copy link
Member Author

On my side this PR is ready for review and merging.

Copy link
Member

@rhugonnet rhugonnet left a comment

Choose a reason for hiding this comment

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

Amazing!!
So it's having inplace: bool = ... repeated in an @overload above the main function definition that made MyPy happy in the end?

@@ -1899,7 +1899,7 @@ def crop(
self._data = crop_img
self.transform = tfm
self.tags["AREA_OR_POINT"] = "Area" # TODO: Explain why this should have an area interpretation now
return None
return self
Copy link
Member

Choose a reason for hiding this comment

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

This should still return None if it is inplace and crop_img was already written to .data, no? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I agree, we should not return self, if the data is modified in place, otherwise this becomes confusing...

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, ok it looks like this snuck through, which was not the intention. I did this when having problems with @overload but that got resolved. Will sort soon, for the moment I'll open a bug

geoutils/raster/raster.py Show resolved Hide resolved
geoutils/raster/raster.py Show resolved Hide resolved
doc/source/raster_class.md Show resolved Hide resolved
@atedstone atedstone merged commit 5d5e269 into GlacioHack:main Jan 18, 2024
13 checks passed
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.

3 participants