Implement getter methods on AffineTransform to access internal elements #1159

weiji14 · 2024-03-12T02:08:10Z

Enable internal elements of the affine transform matrix to be retrieved using getter methods. Included some short documentation and a unit test.

Example usage:

use geo::AffineTransform;

let transform = AffineTransform::new(10.0, 0.0, 400_000.0, 0.0, -10.0, 500_000.0);

let a: f64 = transform.a();
let b: f64 = transform.b();
let xoff: f64 = transform.xoff();
let d: f64 = transform.d();
let e: f64 = transform.e();
let yoff: f64 = transform.yoff();

assert_eq!(transform, AffineTransform::new(a, b, xoff, d, e, yoff))

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Fixes #1158

Enable internal elements of the affine transform matrix to be retrieved using letters "a", "b", "xoff", "d", "e", "yoff". Included some short documentation and a unit test.

weiji14 · 2024-03-12T02:09:56Z

geo/src/algorithm/affine_ops.rs

+            "a" => &self.0[0][0],
+            "b" => &self.0[0][1],
+            "xoff" => &self.0[0][2],
+            "d" => &self.0[1][0],
+            "e" => &self.0[1][1],
+            "yoff" => &self.0[1][2],


Need to decide if using these names are ok, or maybe just do a, b, c, d, e, f. See discussion at #1158.

weiji14 · 2024-03-12T02:10:45Z

geo/src/algorithm/affine_ops.rs

+            "d" => &self.0[1][0],
+            "e" => &self.0[1][1],
+            "yoff" => &self.0[1][2],
+            _ => &f64::NAN,


Not ideal to return NaN here, would prefer if the possible indexes are limited to just 6 possible values, maybe with an enum?

With getters this wouldn't be an issue.

Out-of-bounds indexing in Rust is usually a panic, right? I'm not sure if this is strictly out of bounds, though.

Yeah on second thought, getter methods might actually be better. I've refactored things in commit a0fdea5

lnicola · 2024-03-12T09:36:43Z

I personally prefer methods (getters) instead of Index.

More ergonomic access since it allow type-ahead, and no risk of out of bound errors.

kylebarron · 2024-03-13T00:33:25Z

geo/src/algorithm/affine_ops.rs

    /// ```
    pub fn new(a: T, b: T, xoff: T, d: T, e: T, yoff: T) -> Self {
        Self([[a, b, xoff], [d, e, yoff], [T::zero(), T::zero(), T::one()]])
    }
+
+    /// Get the x-resolution value.
+    pub fn a(&self) -> &T {


CoordNum implements Copy, so is it better to return T here instead?

Yeah, finding out that I need to deref or call .to_owned() on the value in order to use it elsewhere. Maybe it is better to return a cloned copy?

Edit: done at 6431033

since T is copy, you should be able to just remove the & I think

kylebarron · 2024-03-13T00:34:46Z

geo/src/algorithm/affine_ops.rs

+/// let c: f64 = transform.xoff();
+/// let d: f64 = transform.d();
+/// let e: f64 = transform.e();
+/// let f: f64 = transform.yoff();


To make this an actual doctest, you could create a second transform with these values and assert it's equal?

assert_eq!(transform, AffineTransform::new(a, b, c, d, e, f))

Ok, done at 771061c!

kylebarron · 2024-03-13T00:35:04Z

geo/src/algorithm/affine_ops.rs

+///
+/// let a: f64 = transform.a();
+/// let b: f64 = transform.b();
+/// let c: f64 = transform.xoff();


Maybe name this variable binding xoff? Not sure

Yes, xoff please.

Done in a6138f1

urschrei · 2024-03-13T13:21:53Z

geo/src/algorithm/affine_ops.rs

+///
+/// let a: f64 = transform.a();
+/// let b: f64 = transform.b();
+/// let c: f64 = transform.xoff();


Yes, xoff please.

geo/src/algorithm/affine_ops.rs

urschrei · 2024-03-13T13:25:42Z

geo/src/algorithm/affine_ops.rs

+    /// Get the y-offset value.
+    pub fn yoff(&self) -> T {
+        self.0[1][2]
+    }


These doc comments – if they're needed at all – should not refer to the terminology ("resolution", "rotation" etc) used in Matt's blog post, as these methods aren't solely intended for vector <--> raster coordinate transformations.

Yes, the methods here are only used to retrieve the element values, not for coordinate transformation. So what would be good descriptions for each of these getter methods? Do we simply don't assign any meaning to them, and assume that the user can figure out what the a, b, xoff, c, d, yoff values represent?

That depends on whether it's reasonable to assume that someone who's trying to build a custom affine transform is familiar with its mechanics, and has (or is likely to) look at https://docs.rs/geo/latest/geo/algorithm/affine_ops/struct.AffineTransform.html, where the position and thus meaning of a, b, xoff etc are obvious.

Ideally, there should be some description when a user hovers over the method in their IDE, so that they don't have to refer to the online documentation. Having a method with a single letter like .a() or .b() unfortunately isn't very descriptive...

How about just a link back to the constructor docs, which has an ascii diagram of the matrix?

// See [new] for this value's role in the affine transformation.

How about just a link back to the constructor docs, which has an ascii diagram of the matrix?

Tried this locally, and clicking on the AffineTransform::new link in my editor still takes me to the online documentation page.

But I'll take this as a good-enough compromise, unless anyone has any other suggestions?

In the rendered docs online that link should point to the correct anchor.

Work out the list of x and y coordinates for the raster grid from the Affine transformation matrix. Requires changes being developed at georust/geo#1159.

Link to the AffineTransform::new docs for each of the a, b, xoff, d, e, yoff methods, instead of using descriptions like 'x-resolution', 'x-rotation', 'x-offset', 'y-rotation', 'y-resolution', and 'y-offset' respectively.

weiji14 · 2024-05-07T23:42:22Z

Anything else I can do to move this PR along?

michaelkirk

LGTM - and it looks like @urschrei's feedback has been addressed.

Getter methods on AffineTransform implemented in georust/geo#1159 has been merged into the main branch of georust/geo, so no longer need to point to my personal fork. Also fixed a mismatched type when accessing the x_res and y_res attributes.

* ✨ Implement cog3pio xarray BackendEntrypoint Initial implementation of a 'cog3pio' xarray BackendEntrypoint for decoding GeoTIFF files! Following instructions at https://docs.xarray.dev/en/v2024.02.0/internals/how-to-add-new-backend.html on registering a backend. Only a minimal implementation for now to read the pixel array data, with dummy x and y coordinates. * ➕ Add xarray N-D labeled arrays and datasets in Python! * 🌐 Decode x and y coordinates from affine transform Work out the list of x and y coordinates for the raster grid from the Affine transformation matrix. Requires changes being developed at georust/geo#1159. * 👷 Disable CI builds on 32-bit Windows Getting an error with compiling pandas 2.2.1 on Windows x86 (32-bit), and pandas has moved away from providing 32-bit Windows wheels, see pandas-dev/pandas#54979, so might as well not build cog3pio wheels for 32-bit Windows since we can't test them properly. * ⚗️ Benchmark cog3pio engine against rioxarray Parametrized test to compare loading a GeoTIFF using cog3pio vs rioxarray via their respective xarray BackendEntrypoints. Made a new extras dependency group in pyproject.toml called 'benchmark', and listed both `pytest-codspeed` and `rioxarray` under it. * 🐛 Add half pixel offset to get centrepoint of top left pixel The affine transform from the GeoTIFF represents the top-left corner of the top-left pixel (gridline registration), but we need to convert that to the center of the top-left pixel (pixel registration) which is what xarray typically assumes. Added some more comments on how the xy_coords is calculated, and updated unit tests with new x/y min/max bounds. Commented out the mean value assertion for now as NaN handling is not implemented yet. * 📌 Pin to georust/geo at rev 481196b Getter methods on AffineTransform implemented in georust/geo#1159 has been merged into the main branch of georust/geo, so no longer need to point to my personal fork. Also fixed a mismatched type when accessing the x_res and y_res attributes. * 📝 Document usage of cog3pio xarray backend engine in main README.md Show how a GeoTIFF file can be read into an xarray.DataArray object by passing `engine="cog3pio"` to xarray.open_dataarray. * 👥 Push out multi-dtype feature in roadmap to Q2 The multiple dtype feature is a little trickier than expected, so will push this out as a medium term goal for Q2 2024. Likely will depend on how the georust/geotiff crate's progress is like.

Implement Index trait on AffineTransform to access internal elements

f4ca06b

Enable internal elements of the affine transform matrix to be retrieved using letters "a", "b", "xoff", "d", "e", "yoff". Included some short documentation and a unit test.

weiji14 commented Mar 12, 2024

View reviewed changes

weiji14 mentioned this pull request Mar 12, 2024

Expose fields in AffineTransform struct (or have a way to access the internal values)? #1158

Closed

Refactor to access affine transform elements using getter methods

a0fdea5

More ergonomic access since it allow type-ahead, and no risk of out of bound errors.

weiji14 changed the title ~~Implement Index trait on AffineTransform to access internal elements~~ Implement getter methods on AffineTransform to access internal elements Mar 12, 2024

Add changelog entry and fix some typos

0bd04a5

kylebarron reviewed Mar 13, 2024

View reviewed changes

weiji14 added 2 commits March 13, 2024 15:57

Return owned CoordNum instead of reference

6431033

Turn example into a doctest by adding assert comparison

771061c

urschrei requested changes Mar 13, 2024

View reviewed changes

Rename c to xoff and f to yoff in doctest

a6138f1

weiji14 mentioned this pull request Mar 25, 2024

✨ Implement cog3pio xarray BackendEntrypoint weiji14/cog3pio#14

Merged

6 tasks

weiji14 marked this pull request as ready for review April 26, 2024 04:54

kylebarron approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'main' into affinetransform-index

de84485

michaelkirk approved these changes May 8, 2024

View reviewed changes

michaelkirk enabled auto-merge May 8, 2024 01:22

urschrei approved these changes May 8, 2024

View reviewed changes

michaelkirk added this pull request to the merge queue May 8, 2024

Merged via the queue into georust:main with commit 481196b May 8, 2024
15 checks passed

weiji14 deleted the affinetransform-index branch May 8, 2024 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement getter methods on AffineTransform to access internal elements #1159

Implement getter methods on AffineTransform to access internal elements #1159

weiji14 commented Mar 12, 2024 •

edited

Loading

weiji14 Mar 12, 2024

weiji14 Mar 12, 2024

lnicola Mar 12, 2024

urschrei Mar 12, 2024

weiji14 Mar 12, 2024

lnicola commented Mar 12, 2024 •

edited

Loading

kylebarron Mar 13, 2024

weiji14 Mar 13, 2024 •

edited

Loading

kylebarron Mar 13, 2024

kylebarron Mar 13, 2024

weiji14 Mar 13, 2024

kylebarron Mar 13, 2024

urschrei Mar 13, 2024

weiji14 Mar 13, 2024

urschrei Mar 13, 2024

urschrei Mar 13, 2024

weiji14 Mar 13, 2024

urschrei Mar 13, 2024

weiji14 Mar 13, 2024

michaelkirk Apr 24, 2024

weiji14 Apr 26, 2024

kylebarron Apr 26, 2024 •

edited

Loading

weiji14 commented May 7, 2024

michaelkirk left a comment

Implement getter methods on AffineTransform to access internal elements #1159

Implement getter methods on AffineTransform to access internal elements #1159

Conversation

weiji14 commented Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnicola commented Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

weiji14 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylebarron Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

weiji14 commented May 7, 2024

michaelkirk left a comment

Choose a reason for hiding this comment

weiji14 commented Mar 12, 2024 •

edited

Loading

lnicola commented Mar 12, 2024 •

edited

Loading

weiji14 Mar 13, 2024 •

edited

Loading

kylebarron Apr 26, 2024 •

edited

Loading