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

Add support for int64, uint64, and int8 #353

Merged
merged 11 commits into from Oct 6, 2023
Merged

Add support for int64, uint64, and int8 #353

merged 11 commits into from Oct 6, 2023

Conversation

emlys
Copy link
Member

@emlys emlys commented Sep 26, 2023 •
edited

Fixes #352
Fixes #329

  • Removed our mappings between numpy and gdal dtypes, using the functions gdal provides for this instead. That way we don't have to explicitly include different dtypes depending on the gdal version.
  • Bump up the GDAL and python versions that we test in github actions
  • Skip a couple of tests related to signed bytes on GDAL>=3.7. Since the new gdal.GDT_Int8 is just like any other GDAL type, I don't think we need to add tests specifically for it.
  • Fixed what seems like a bug in create_raster_from_base, where dtype-related metadata (PIXELTYPE=SIGNEDBYTE) was copied over from the base raster, even though the target dtype is not copied from the base raster. Now that metadata has to be set in the raster_creation_tuple.

behavior of GDAL 3.7+ with signed bytes

A warning is raised if you create a raster using the deprecated flag:

>>> driver = gdal.GetDriverByName('GTIFF')
>>> raster = driver.Create('foo.tif', 1, 1, 1, gdal.GDT_Byte, options=['PIXELTYPE=SIGNEDBYTE'])
Warning 1: Using PIXELTYPE=SIGNEDBYTE with Byte data type is deprecated (but still works). Using Int8 data type instead is now recommended.

It seems that drivers will still be able to read signed byte rasters. They just treat them as a new data type (Int8) and don't return the PIXELTYPE=SIGNEDBYTE metadata element:

>>> band = raster.GetRasterBand(1)
>>> band.DataType
14  # gdal.GDT_Int8
>>> band.GetMetadata('IMAGE_STRUCTURE')
{}

From the migration guide,

Following RFC 87, PIXELTYPE=SIGNEDBYTE in IMAGE_STRUCTURE metadata domain is
no longer reported by drivers that used to do it. The new GDT_Int8 data type
is now reported.
On writing, the PIXELTYPE=SIGNEDBYTE creation option is preserved in drivers
that used to support it, but is deprecated and external code should rather use
the GDT_Int8 data type.

@emlys emlys changed the title add support for int64 and uint64 Add support for int64, uint64, and int8 Oct 5, 2023
@emlys emlys marked this pull request as ready for review October 5, 2023 21:24
@emlys emlys requested a review from phargogh October 5, 2023 21:24
phargogh
phargogh requested changes Oct 5, 2023
View reviewed changes
Copy link
Member

@phargogh phargogh left a comment

Choose a reason for hiding this comment

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

This is great! Thanks @emlys . I just had a couple comments on docstrings in the new functions and a question about how GDAL handles duplicate creation options.

pyproject.toml
@@ -13,8 +13,7 @@ build-backend = "setuptools.build_meta"

[tool.pytest.ini_options]
# Raise warnings to exceptions.
filterwarnings = 'error'

filterwarnings = ["error", "default::DeprecationWarning", "default::FutureWarning"]
Copy link
Member

Choose a reason for hiding this comment

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

Nice. This is a great idea and I'm glad they're supporting this in pyproject.toml now.

src/pygeoprocessing/geoprocessing.py
@@ -3703,7 +3665,7 @@ def mask_op(base_array, mask_array):
os.remove(mask_raster_path)


def _gdal_to_numpy_type(band):
def _gdal_to_numpy_type(gdal_type, metadata):
Copy link
Member

Choose a reason for hiding this comment

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

Since the function interface is changing, could you update the docstring here as well?

src/pygeoprocessing/geoprocessing.py
('PIXELTYPE' in metadata and metadata['PIXELTYPE'] == 'SIGNEDBYTE'))):
return numpy.int8

numpy_type = gdal_array.GDALTypeCodeToNumericTypeCode(gdal_type)
Copy link
Member

Choose a reason for hiding this comment

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

Well how about that! This is a nice function to know about.

src/pygeoprocessing/geoprocessing.py

if band.DataType != gdal.GDT_Byte:
raise ValueError("Unsupported DataType: %s" % str(band.DataType))
def _numpy_to_gdal_type(numpy_type):
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a docstring here that includes the input and output types? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a docstring!

src/pygeoprocessing/symbolic.py
geoprocessing._numpy_to_gdal_type(target_numpy_type))

driver, creation_options = raster_driver_creation_tuple
creation_options = list(creation_options) + type_creation_options
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to handle the case where a creation option is defined both in creation_options and in type_creation_options? Will GDAL work as we hope if an option is defined in both?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to work as expected if PIXELTYPE=SIGNEDBYTE is defined twice. And it seems that SIGNEDBYTE is the only recognized value for PIXELTYPE, so it wouldn't overwrite a different option.

src/pygeoprocessing/geoprocessing_core.pyx
Comment on lines +659 to +661
numpy_type = pygeoprocessing.get_raster_info(
base_raster_path_band[0])['numpy_type']
if numpy.issubdtype(numpy_type, numpy.integer):
Copy link
Member

Choose a reason for hiding this comment

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

This is a really nice update - one less thing to have to worry about updating if GDAL adds more int or float types!

@emlys emlys requested a review from phargogh October 6, 2023 18:27
@phargogh
Copy link
Member

phargogh commented Oct 6, 2023

Looks great! Just waiting for the tests to finish before merging.

@phargogh phargogh merged commit 66a1f59 into natcap:main Oct 6, 2023
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phargogh phargogh phargogh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for int64 and uint64 GDAL 3.7 changes behavior for signed bytes
2 participants
@emlys @phargogh