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

Modify set_nodata behaviour, improve related warnings and tests #309

Merged
merged 20 commits into from
Sep 16, 2022

Conversation

rhugonnet
Copy link
Member

@rhugonnet rhugonnet commented Sep 14, 2022

This PR homogenizes the getter and setter functionality related to nodata and adds more exhaustive tests.

In details, this PR:

  • Renames set_ndv() into set_nodata(), and all other occurences of "ndv" to "nodata", in order to match the nodata naming of rasterio and avoid the confusion of having "ndv" and "nodata" standing for the same thing,
  • Corrects the behaviour of update_array in set_nodata(),
  • Adds the update_mask argument to set_nodata(),
  • Adds a nodata.setter method that calls set_nodata() with default parameters,
  • Improves or adds tests for set_nodata and nodata.setter,
  • Moves the nodata-related functions/class methods together in the file (among others, mypy request the getter/setter method to follow each other).

The old behaviour was:

  • The argument update_array was False by default,
  • The argument update_array would modify .data.mask by unmasking old nodata and masking new nodatas (i.e. a rare case where the nodata value was wrongly defined and needed to be changed while ignoring old nodatas).

The new behaviour is:

  • Both arguments update_array and update_mask are True by default,
  • The argument update_array updates the array .data.data by replacing old nodata values into new nodata values.
  • The argument update_mask updates the mask .data.mask by unmasking old nodata and masking new nodatas (including the ones possibly updated by update_array, i.e. essentially keeping the mask on old nodata that have been updated, which makes the big difference with the previous behaviour).
  • A UserWarning is raised if the new nodata value already exist as a valid value of the array (as it will be automatically masked).

This behaviour is also explained in the function description, with typical use-cases.

Resolves #286

@rhugonnet rhugonnet marked this pull request as draft September 14, 2022 13:49
@rhugonnet rhugonnet marked this pull request as ready for review September 15, 2022 12:21
@rhugonnet rhugonnet requested a review from adehecq September 15, 2022 12:45
@adehecq
Copy link
Member

adehecq commented Sep 15, 2022

It's great to have everything a bit more consistent now ! Thanks ! 👏
I guess this will call for some changes in xdem...

* Renames `set_ndv()` into `set_nodata()`, and all other occurences of "ndv" to "nodata", in order to match the `nodata` naming of `rasterio` and avoid the confusion of having "ndv" and "nodata" standing for the same thing,

A good way to increase your git blame score 😝

* Adds a `nodata.setter` method that calls `set_nodata()` with default parameters,

See my comment, I would simply raise an error and tell the user to use set_nodata, as it will force the use to think about the consequences.

@rhugonnet
Copy link
Member Author

Actually, I already checked, and there's not a single function using this in xDEM! 🥳 Except what I just added to remove the floating nodata in the ddem example.

@rhugonnet
Copy link
Member Author

rhugonnet commented Sep 15, 2022

But I'm quite happy! data and nodata are our two big properties of GeoUtils, intertwined one with another, and we finally have them consistent with most common and intuitive behaviour, and with descriptions for other options! 🙂

The change in the function themselves is not so impressive, but our test suite has gained nearly 1000 lines to ensure all is robust! And there were a lot of weird behaviours before those were corrected with the new tests! 🙌

Copy link
Member

@adehecq adehecq 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, except for a minor comment to one test

@rhugonnet rhugonnet merged commit 9386dbf into GlacioHack:main Sep 16, 2022
@rhugonnet rhugonnet deleted the update_setndv branch September 16, 2022 07:48
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.

set_ndv does not update nodatas in array
2 participants