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.

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

ENH: Add CoordTransform::transform_bounds() #272

Merged
merged 5 commits into from May 24, 2022

Conversation

brendan-ward
Copy link
Contributor

@brendan-ward brendan-ward commented May 12, 2022

  • 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.

This wraps OCTTransformBounds to provide CoordTransform::transform_bounds() for GDAL >= 3.4

Output values were verified manually using rasterio.

@brendan-ward
Copy link
Contributor Author

I'd like to add an example to examples/spatial_reference.rs, but it is unclear how to do so for GDAL version-specific functions; I did not see other version-specific functionality in any of the examples. Is there a pattern for doing so, or it better to leave out such examples?

@brendan-ward brendan-ward marked this pull request as ready for review May 12, 2022 23:48
Ok([out_xmin, out_ymin, out_xmax, out_ymax])
} else {
let err = _last_cpl_err(CPLErr::CE_Failure);
let msg = if let GdalError::CplError { msg, .. } = err {
Copy link
Member

Choose a reason for hiding this comment

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

A match might look nicer here (you can probably drop the trim), but it probably doesn't matter too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was copy-pasted from transform_coords for consistency; ideally both would get updated to be the same if match is more appropriate. I can update just this one here and the other in a follow up PR, or both in a follow up PR.

I'd probably also want to invert the logic a little bit so that only the error case is in the conditional and have a bare Ok:

if !ret_val {...handle error and return... }

Ok([out_xmin, out_ymin, out_xmax, out_ymax])

But didn't do so in this PR out of consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I can update just this one here and the other in a follow up PR, or both in a follow up PR.

Right, it's fine either way.

@lnicola
Copy link
Member

lnicola commented May 13, 2022

Test failure seems unrelated:

---- config::tests::test_error_handler stdout ----
thread 'config::tests::test_error_handler' panicked at 'assertion failed: `(left == right)`
  left: `[(Failure, 42, "foo"), (Warning, 1, "bar"), (Warning, 1, "GPKG: bad application_id=0x00000000 on '/tmp/.tmp25HDOI.gpkg'")]`,
 right: `[(Failure, 42, "foo"), (Warning, 1, "bar")]`', src/config.rs:230:9

/// None if there is an error.
#[cfg(all(major_ge_3, minor_ge_4))]
pub fn transform_bounds(&self, bounds: &[f64; 4], densify_pts: i32) -> Result<([f64; 4])> {
let mut out_xmin: f64 = 0.;
Copy link
Member

Choose a reason for hiding this comment

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

You'll have to move the use here to avoid the warning. Or you might be able to skip the casts, like in transform_coords, I'm not sure.

@lnicola
Copy link
Member

lnicola commented May 21, 2022

Still a formatting error (try cargo fmt).

@lnicola
Copy link
Member

lnicola commented May 23, 2022

LGTM, I'll merge this tomorrow unless anyone else has objections.

@lnicola
Copy link
Member

lnicola commented May 24, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented May 24, 2022

Build succeeded:

@bors bors bot merged commit fb6e260 into georust:master May 24, 2022
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.

None yet

2 participants