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 inconsistent handling of RasterSourceConfig.bbox's type #1899

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

AdeelH
Copy link
Collaborator

@AdeelH AdeelH commented Sep 5, 2023

Overview

This PR makes it so that RasterSourceConfig.bbox is always a tuple, to allow it to be serialized and deserialized by pydantic.

Motivation: previously, RasterSourceConfig.bbox was defined as a tuple in the RasterSourceConfig definition, but was automatically converted to a Box in its pydantic validator. This meant that although the field could be initialized as a Box, it could not be serialized by pydantic. This was not caught in the integration tests, because none of them use the bbox field.

The error can be reproduced by e.g. adding bbox=(0, 0, 328, 1342) to RasterioSourceConfig the chip_classification.basic integration test.

Checklist

  • Added needs-backport label if PR is bug fix that applies to previous minor release
  • Ran scripts/format_code and committed any changes
  • Documentation updated if needed
  • PR has a name that won't get you publicly shamed for vagueness

Notes

This PR is an alternative to #1895.

Testing Instructions

  • See new unit tests.
  • Check if chip_classification.basic integration test runs with bbox=(0, 0, 328, 1342) added to RasterioSourceConfig.

@AdeelH AdeelH added the needs-backport This PR needs to be backported to release branches label Sep 5, 2023
@AdeelH
Copy link
Collaborator Author

AdeelH commented Sep 5, 2023

@jamesmcclain please check if this fix works as well as #1895.

@jamesmcclain
Copy link
Contributor

@jamesmcclain please check if this fix works as well as #1895.

I'll give it a try first thing tomorrow 👍

It is now always a tuple that is should be converted to a Box when needed.
@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #1899 (9dc948f) into master (6249e01) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1899      +/-   ##
==========================================
- Coverage   81.56%   81.56%   -0.01%     
==========================================
  Files         190      190              
  Lines        9283     9282       -1     
==========================================
- Hits         7572     7571       -1     
  Misses       1711     1711              
Files Changed Coverage Δ
...on/core/data/raster_source/raster_source_config.py 48.00% <ø> (-10.07%) ⬇️
...e/data/raster_source/multi_raster_source_config.py 81.39% <100.00%> (+0.90%) ⬆️
.../core/data/raster_source/rasterio_source_config.py 56.52% <100.00%> (+4.14%) ⬆️
...ore/data/raster_source/rasterized_source_config.py 100.00% <100.00%> (ø)

@jamesmcclain
Copy link
Contributor

@jamesmcclain please check if this fix works as well as #1895.

I'll give it a try first thing tomorrow 👍

@AdeelH Yes, it works 👍 👍 . Many thanks for this 🙏

@AdeelH AdeelH marked this pull request as ready for review September 6, 2023 15:02
@AdeelH AdeelH merged commit 1c9cc25 into azavea:master Sep 6, 2023
1 check passed
@AdeelH AdeelH deleted the rs_cfg_bbox branch September 6, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-backport This PR needs to be backported to release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants