Skip to content

Conversation

@gonzmg88
Copy link
Collaborator

@gonzmg88 gonzmg88 commented Apr 24, 2025

This PR has many new features for the GeoTensor class. In particular GeoTensor now inherits from np.ndarray and therefore impllements most of numpy API.

In theory the only breaking change is the one mention below. Since GeoTensor is a central class of the package, I decided to create a new major version.

See GeoTensor module docs and GeoTensor numpy API tutorial for detailed example of new features.

Alpha docs of this version at: https://spaceml-org.github.io/georeader/alpha/

Breaking changes

  • We do not support anymore torch.Tensor as inner value for GeoTensor, I don't think it was a feature used.

These changes are already as an alpha version in pypi. In order to install this alpha version:

pip install georeader-spaceml --pre

Pending numpy API features

These functions will work but the propagated transform with not be correct:

  • reshape to allow flatteing time and band axis.
  • rotations (e.g. np.rot90)
  • np.transpose to allow transposing x and y axes (and transposing the transform)
  • np.flip , np.rollaxis, np.moveaxis....

@gonzmg88 gonzmg88 requested a review from Copilot April 24, 2025 08:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces version 2.0 of the GeoTensor functionality with added support for the NumPy API and several documentation improvements. Key changes include:

  • Updating the package version in pyproject.toml to 2.0.0a2.
  • Revising navigation and content in the documentation (mkdocs.yml, various markdown files) to reflect the new GeoTensor features and usage examples.
  • Updating example code in docs/index.md, README.md, and related files to use RasterioReader for Sentinel-2 imagery instead of the previously used S2_SAFE_reader.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pyproject.toml Version bump update
mkdocs.yml Added new navigation entries for GeoReader concepts and new modules
docs/modules/read_module.md Updated API sections and headings
docs/modules/plot_module.md New module documentation for plotting functions
docs/modules/geotensor_module.md Expanded documentation for GeoTensor and its NumPy API support
docs/index.md Revised example code and updated imagery source details
docs/Sentinel-2/run_in_gee_image.ipynb Added installation instructions and updated Python version info
README.md Revised example code and updated imagery source details
Comments suppressed due to low confidence (3)

docs/index.md:35

  • Typo: 'croping' should be corrected to 'cropping'.
See also read.read_from_bounds, read.read_from_polygon for different ways of croping an image

README.md:35

  • Typo: 'croping' should be corrected to 'cropping'.
See also read.read_from_bounds, read.read_from_polygon for different ways of croping an image

docs/modules/geotensor_module.md:5

  • Typo: 'subseting' should be corrected to 'subsetting'.
allow for operations like reprojection, subseting from spatial coordinates, mosaicking or vectorization.

@gonzmg88 gonzmg88 requested a review from Copilot May 8, 2025 10:41
@gonzmg88 gonzmg88 marked this pull request as draft May 8, 2025 10:41
@gonzmg88 gonzmg88 marked this pull request as draft May 8, 2025 10:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements GeoTensor version 2.0 with a refactored numpy API for GeoTensor, along with several documentation updates and minor internal logic adjustments.

  • Update package version and breaking changes in pyproject.toml
  • Adjust navigation and documentation to highlight new GeoTensor and API features
  • Modify vectorization and plotting functions to support new masking and extent calculations

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pyproject.toml Updated version to "2.0.0a3" reflecting the new major release
mkdocs.yml Added new navigation items for updated modules and plotting docs
georeader/vectorize.py Changed type checking logic for handling GeoData vs numpy arrays
georeader/plot.py Revised extent calculation for non-rectilinear transforms and mask handling
docs/* Updated documentation to include new GeoTensor API and usage examples
Makefile Added target to publish alpha docs under a separate documentation path

Comment on lines +27 to 33
if not hasattr(binary_mask, "transform"):
binary_mask_np = binary_mask
else:
binary_mask_np = binary_mask.values
binary_mask_np = np.array(binary_mask)

assert transform is None, "transform only must be used if input is np.ndarray"
transform = binary_mask.transform
Copy link

Copilot AI May 8, 2025

Choose a reason for hiding this comment

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

Using hasattr() here as a replacement for isinstance(binary_mask, np.ndarray) might not capture all intended types. Consider confirming that this condition correctly distinguishes raw numpy arrays from GeoData objects.

Copilot uses AI. Check for mistakes.
if max_val_mask is None:
max_val_mask = color_array.shape[0]

assert (max_val_mask - min_val_mask) == color_array.shape[0], f"max_val_mask - min_val_mask must be equal to the number of colors {max_val_mask} - {min_val_mask} != {color_array.shape[0]}"
Copy link

Copilot AI May 8, 2025

Choose a reason for hiding this comment

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

Using 'assert' for validating the mask range could be problematic if assertions are disabled in production environments. Consider raising a ValueError with a clear error message instead.

Copilot uses AI. Check for mistakes.
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.

6 participants