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.

Sign up for GitHub

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

Jump to bottom

Allow Rasters as inputs for Coreg.fit() and Coreg.apply() #175

Merged
merged 4 commits into from
Jul 5, 2021
Merged

Allow Rasters as inputs for Coreg.fit() and Coreg.apply() #175

merged 4 commits into from
Jul 5, 2021

Conversation

erikmannerfelt
Copy link
Contributor

Solves #174.

See the issue for the new syntax!

@erikmannerfelt erikmannerfelt added the enhancement Feature improvement or request label Jul 3, 2021
@erikmannerfelt erikmannerfelt self-assigned this Jul 3, 2021
@erikmannerfelt erikmannerfelt linked an issue Jul 3, 2021 that may be closed by this pull request
Allow Raster (sub-)classes as inputs for Coreg.fit() and Coreg.apply() #174
Closed
@erikmannerfelt erikmannerfelt mentioned this pull request Jul 3, 2021
Closed
rhugonnet
rhugonnet reviewed Jul 3, 2021
View reviewed changes
xdem/coreg.py Outdated Show resolved Hide resolved
rhugonnet
rhugonnet reviewed Jul 3, 2021
View reviewed changes
tests/test_coreg.py
@@ -379,7 +380,7 @@ def test_z_scale_corr(self):
zcorr.fit(self.ref.data, scaled_dem)

# Apply the correction
unscaled_dem = zcorr.apply(scaled_dem, None)
unscaled_dem = zcorr.apply(scaled_dem, self.ref.transform)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I get why those needed changing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raster.apply() now raises an error if an array is provided but no transform. This makes the tools perfectly consistent (there was no previous mechanism to validate that a transform was given, so the error messages were very confusing). In this particular case, it's a bit redundant, but I think that's an okay sacrifice for consistency! If you and @adehecq disagree, I'm sure we can find another way.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds ok to me

rhugonnet
rhugonnet previously approved these changes Jul 3, 2021
View reviewed changes
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.

Tip top!
Birds and heat have a much more productively-positive effect on you than me 😆

@rhugonnet
Copy link
Member

Actually, a big question raised by this:
With GlacioHack/geoutils#210 and this PR, we now have many use cases where we can use np.ndarray and Raster objects interchangeably. But there is still several functions for which it does not work, right? Such as terrain attributes.

Should we aim to have xdem working with np.ndarrays and Raster objects interchangeably? If yes, what is left to do? (I think that, @erikmannerfelt, you have the best viewpoint on all this!)
Also, it should be crystal clear when a Raster can be provided simply as a convenience and a np.ndarray would suffice, or when georeferenced data is absolutely necessary. It will get blurry for the users if we allow to provide both everywhere for convenience, so I'm not sure how to tackle this!

@erikmannerfelt
Copy link
Contributor Author

Actually, a big question raised by this:
With GlacioHack/GeoUtils#210 and this PR, we now have many use cases where we can use np.ndarray and Raster objects interchangeably. But there is still several functions for which it does not work, right? Such as terrain attributes.

Should we aim to have xdem working with np.ndarrays and Raster objects interchangeably? If yes, what is left to do? (I think that, @erikmannerfelt, you have the best viewpoint on all this!)
Also, it should be crystal clear when a Raster can be provided simply as a convenience and a np.ndarray would suffice, or when georeferenced data is absolutely necessary. It will get blurry for the users if we allow to provide both everywhere for convenience, so I'm not sure how to tackle this!

Since, geoutils==0.0.3, Rasters implement the array interface, so they should be possible to use interchangeably with ndarrays. The tests don't cover that "should" part, however, and Rasters are not consistently returned if provided as an argument. For now, all functions that show a Raster/RasterType argument/return type hint returns a RasterType if given a RasterType, and those who don't will return an ndarray/masked_array.

Long story short, the only way to validate how interchangeable they are is to include more tests!!

@erikmannerfelt
Merge branch 'main' of github.com:GlacioHack/xdem into coreg-allow-ra…
cc0cdb3 
…ster-classes

Added warnings for overridden 'transform' argument.
Improved argument validation for 'Coreg'
@erikmannerfelt erikmannerfelt mentioned this pull request Jul 3, 2021
Open
@erikmannerfelt
Copy link
Contributor Author

I think I've fixed all of @rhugonnet 's comments.

Any other thoughts, @rhugonnet and @adehecq ?

rhugonnet
rhugonnet approved these changes Jul 5, 2021
View reviewed changes
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.

All good for me!
Maybe we should open an issue on the "interchangeable" discussion?
It would be good if we could say "All xdem functions accept interchangeably np.ndarray and Raster subclass objects"

@erikmannerfelt
Copy link
Contributor Author

Already done in #178 !

Thanks, @rhugonnet !

@erikmannerfelt erikmannerfelt merged commit a12f0a8 into GlacioHack:main Jul 5, 2021
@erikmannerfelt erikmannerfelt deleted the coreg-allow-raster-classes branch July 7, 2021 10:43
@adehecq
Copy link
Member

adehecq commented Jul 27, 2021

Fantastic !

This was referenced Jul 28, 2021
Merged
Closed
@adehecq adehecq mentioned this pull request Nov 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adehecq adehecq adehecq left review comments

@rhugonnet rhugonnet rhugonnet approved these changes

Assignees

@erikmannerfelt erikmannerfelt

Labels
enhancement Feature improvement or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow Raster (sub-)classes as inputs for Coreg.fit() and Coreg.apply()
3 participants
@erikmannerfelt @rhugonnet @adehecq