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

fix: store raster nodata value in table metadata #111

Merged

Conversation

volaya
Copy link
Contributor

@volaya volaya commented Apr 18, 2023

Issue

Fixes #

https://app.shortcut.com/cartoteam/story/307121/handle-nodata-values-in-raster-loader

Proposed Changes

This PR adds the nodata value of the raster layer to the table metadata

Pull Request Checklist

  • [ x] I have tested the changes locally
  • [ x] I have added tests to cover my changes (if applicable)
  • I have updated the documentation (if applicable)

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #307121: Handle nodata values in raster-loader.

@volaya volaya requested a review from vdelacruzb April 18, 2023 06:18
@@ -585,6 +589,7 @@ def rasterio_windows_to_records(
if output_quadbin:
rec = array_to_quadbin_record(
raster_dataset.read(band, window=window),
raster_dataset.nodata,
Copy link
Contributor

Choose a reason for hiding this comment

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

if there is no "nodata" records in a raster what would happen?

The current solution only covers the first time a raster table it's uploaded. We can append tables, in that case to me it's ok taking any of both nodata fields the new one or the old one. If they are not the same the user will have problems though. Something similar happens with the field resolution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there aren't any nodata pixels, the nodata value can be defined anyway (in some file formats is mandatory to define it, in other it is not)

If it is not defined in the file, rasterio returns null, so the metadata would have null. We can decide if, in that case, we want to keep it like that, or maybe not add the "nodata" property to the metadata object

Copy link
Contributor

Choose a reason for hiding this comment

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

that is what i was wondering if there are situations where we don't introduce the nodata. Then we should at least take care of it when appending tables. In the function write_metadata you will see that there are two cases, append_records or not, for now maybe you are testing by using --overwrite so the process is never appending data

Copy link
Contributor

@vdelacruzb vdelacruzb Apr 18, 2023

Choose a reason for hiding this comment

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

This topic as been discussed and nodata is introduced by row. So this implementation makes sense. It's not a global metadata field that we have to append.

Copy link
Contributor

@vdelacruzb vdelacruzb left a comment

Choose a reason for hiding this comment

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

LGTM

@volaya volaya merged commit 5a39f4b into main Apr 18, 2023
@Jesus89 Jesus89 deleted the feature/sc-307121/handle-nodata-values-in-raster-loader branch April 18, 2023 12:36
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.

2 participants