Skip to content

Replace cython _ManagedRaster with shared C++ ManagedRaster implementation#443

Merged
phargogh merged 24 commits intonatcap:mainfrom
emlys:task/439
Jul 16, 2025
Merged

Replace cython _ManagedRaster with shared C++ ManagedRaster implementation#443
phargogh merged 24 commits intonatcap:mainfrom
emlys:task/439

Conversation

@emlys
Copy link
Copy Markdown
Member

@emlys emlys commented Jul 11, 2025

Fixes #439

  • Copied over C++ ManagedRaster extension from natcap.invest
  • Created a new cython module pygeoprocessing.extensions which exposes the C++ extensions ManagedRaster, LRUCache, FastFileIterator, and associated utilities
  • Updated the setup.py configuration to support the new C++ extension (largely copied from natcap.invest's setup.py). This includes adding a new required environment variable when building on Windows, PYGEOPROCESSING_GDAL_LIB_PATH.
  • Replace Cython implementations of ManagedRaster with the new C++ implementation
  • Replace float comparison functions with the new shared function is_close
  • Use shared definitions of some module-level variables rather than re-defining in each module
  • Ensure that all ManagedRasters are closed using the .close() method

When these changes are released, we can remove the ManagedRaster implementation from natcap.invest and instead import it from pygeoprocessing.

# ghi
e = dem_array[row_index, col_index]
if _eq(e, dem_nodata):
if is_close(e, dem_nodata):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

_eq did not allow for float imprecision, but it seems like it should have.

@emlys emlys requested a review from phargogh July 15, 2025 02:04
@emlys emlys self-assigned this Jul 15, 2025
@emlys emlys marked this pull request as ready for review July 15, 2025 02:04
# on push, this is refs/heads/<branch name>
# on pull request, this is refs/pull/<pull request number>/merge
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copied this over from natcap.invest. Keeps queued jobs from building up.

Copy link
Copy Markdown
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.

Makes sense to me, thanks @emlys !

By the way, I know this isn't a user-facing change, but since it is a significant change, do you think we should make a note in HISTORY about it?

@emlys
Copy link
Copy Markdown
Member Author

emlys commented Jul 16, 2025

Sure! Added a history note

@emlys emlys requested a review from phargogh July 16, 2025 15:58
Copy link
Copy Markdown
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.

Thanks @emlys !

@phargogh phargogh merged commit 7cb1003 into natcap:main Jul 16, 2025
93 checks passed
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.

Migrate natcap.invest.managed_raster to pygeoprocessing

2 participants