GITC-7208: Fixing Colormap Rasterization issues #50
Conversation
… docker build process for GDAL
…pects ndv when read from input colormap
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
This looks good, but I'm probably not the best person to approve.
I would like to see a CHANGELOG entry and a bump to the service version file. But if you're not releasing this change it should still have a CHANGELOG with ## [unreleased] - 2025-04-25 or something similiar.
I also forgot to mention, this will almost certainly require updates to the hybig regression tests
docker/service.Dockerfile
Outdated
| -r pip_requirements.txt \ | ||
| -r pip_requirements_skip_snyk.txt | ||
| -r pip_requirements.txt | ||
| # -r pip_requirements_skip_snyk.txt |
There was a problem hiding this comment.
I ❤️ this change to the GDAL dependency. I think you could delete this line and remove that file.
hybig/browse.py
Outdated
| @@ -155,6 +155,7 @@ def create_browse_imagery( | |||
| xml file. | |||
|
|
|||
| """ | |||
| logger.info(message) | |||
There was a problem hiding this comment.
I think this will spit credentials into the log. I don't know if that's important or not. I do this for debugging all the time and I think it's super helpful to have the full message available for debugging.
I have been meaning to send a patch over to the harmony-service-lib to redact that. But it's not there yet.
I don't have strong opinions though, the logs are pretty secure. I noticed this was my idea, it should come out probably.
hybig/browse.py
Outdated
| def get_colormap_from_scaled_colors(scaled_colors) -> dict: | ||
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
I don't remember why I started this, but it can probably be removed.
| ndv_tuple: tuple[float, ...] = dataset.get_nodatavals() | ||
| if ndv_tuple is not None and len(ndv_tuple) > 0: | ||
| # this service only supports one ndv, so just use the first one | ||
| # (usually the only one) | ||
| ds_cmap['nv'] = ds_cmap[ndv_tuple[0]] | ||
| # then remove the value associated with the ndv key | ||
| ds_cmap.pop(ndv_tuple[0]) | ||
| return convert_colormap_to_palette(ds_cmap) |
There was a problem hiding this comment.
This looks like you know more about where color tables and nodata values might be, but if there was a test for it, it might be easier for me to understand.
| TRANSPARENT = np.uint8(0) | ||
| OPAQUE = np.uint8(255) | ||
| TRANSPARENT = uint8(0) | ||
| OPAQUE = uint8(255) |
There was a problem hiding this comment.
This is unrelated to this pr but the typing below for remove_alpha is incorrect.
def remove_alpha(raster: np.ndarray) -> tuple[np.ndarray, np.ndarray, None]:should be:
def remove_alpha(raster: np.ndarray) -> tuple[np.ndarray, np.ndarray | None]:
| rgba_image_slice = scalar_map.to_rgba(band[row_no, :], bytes=True) | ||
| rgba_image[row_no, :, :] = rgba_image_slice | ||
| # Scale input data from 0 to 254 | ||
| normalized_data = norm(band) * 254.0 |
There was a problem hiding this comment.
Probably should add a comment to explain why this uses 254 and not 255.
| band_count: Number of bands in original input data | ||
|
|
||
| The function handles two special cases: | ||
| - JPEG output with 4-band data -> Drop alpha channel and return 3-band RGB |
There was a problem hiding this comment.
Are we dropping handling of this JPEG special case? I think we need to support it for products like PACE.
for more information, see https://pre-commit.ci
Description
This fix is intended to address behavior where HyBIG used to "compress" the colormap of input products by removing unused color levels from the input colormap in the output raster. Now, the service will preserve the colormap information from the input granule if present in the metadata, or provided externally. Additionally, HyBIG will respect the nodata value of the input granule, ensuring that it is represented with a transparency in the output raster.
This is intended to address two issues (see below): the OPERA DSWx-S1 rasterization issue and a similar rasterization "artifact" bug that was previously opened.
Jira Issue ID
GITC-7189
GITC-7208
Local Test Steps
PR Acceptance Checklist
CHANGELOG.mdupdated to include high level summary of PR changes.docker/service_version.txtupdated if publishing a release.