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

feat: IDSSE-256: GridProj #5

Merged
merged 20 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
b2c059b
create grid_proj and unit tests
mackenzie-grimes-noaa Aug 2, 2023
6fdc41c
cleanup flake8 warnings
mackenzie-grimes-noaa Aug 2, 2023
58bbd45
cleanup scattered literal numbers in test_grid_proj, round pixels to int
mackenzie-grimes-noaa Aug 3, 2023
5d216eb
add optional rounding param to map_geo_to_pixel which controls roundi…
mackenzie-grimes-noaa Aug 3, 2023
c60d13b
fix docstring in map_geo_to_pixel
mackenzie-grimes-noaa Aug 3, 2023
89d7611
add round_half_up() utility function, unit tests
mackenzie-grimes-noaa Aug 11, 2023
9a4c6eb
add tests for Map class, exec_cmd, dict_copy_with
mackenzie-grimes-noaa Aug 11, 2023
e6776d9
add unit test for Map __setattr__
mackenzie-grimes-noaa Aug 11, 2023
6b7350f
Merge branch 'feat/idsse-306/round-half-up' into feat/idsse-246/grid-…
mackenzie-grimes-noaa Aug 11, 2023
e7c7be4
add _round_pixel_maybe to grid_proj to standardize pixel rounding
mackenzie-grimes-noaa Aug 11, 2023
9578861
fix grid_proj unit tests
mackenzie-grimes-noaa Aug 11, 2023
8aa1685
round_half_away() now uses only math ceil/floor, no Decimal
mackenzie-grimes-noaa Aug 15, 2023
1dd8a63
Merge branch 'main' into feat/idsse-306/round-half-up
mackenzie-grimes-noaa Aug 15, 2023
252f9c9
cleanup pylint warnings that are my fault
mackenzie-grimes-noaa Aug 15, 2023
8c574f0
Merge branch 'feat/idsse-306/round-half-up' into feat/idsse-246/grid-…
mackenzie-grimes-noaa Aug 15, 2023
d60c7ce
fix rounding tests in test_grid_proj, pylitn warnings
mackenzie-grimes-noaa Aug 15, 2023
9de285e
more specific type hints in grid_prjo
mackenzie-grimes-noaa Aug 15, 2023
63618b0
make _round_pixel_maybe a static method
mackenzie-grimes-noaa Aug 18, 2023
ce4c78d
remove unused imports
mackenzie-grimes-noaa Aug 18, 2023
b287da4
fix all test_grid_proj rounding to use correct round function
mackenzie-grimes-noaa Aug 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions python/idsse_common/idsse/common/aws_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ def aws_cp(self, path: str, dest: str) -> bool:
finally:
pass


def check_for(self, issue: datetime, valid: datetime) -> Optional[Tuple[datetime, str]]:
"""Checks if an object passed issue/valid exists

Expand All @@ -99,8 +98,8 @@ def check_for(self, issue: datetime, valid: datetime) -> Optional[Tuple[datetime
valid (datetime): The valid date/time used to format the path to the object's location

Returns:
Optional[Tuple[datetime, str]]: A tuple of the valid date/time (indicated by object's
location) and location (path) of a object, or None
Optional[Tuple[datetime, str]]: A tuple of the valid date/time (indicated by object's
location) and location (path) of a object, or None
if object does not exist
"""
lead = TimeDelta(valid-issue)
Expand Down Expand Up @@ -161,7 +160,7 @@ def get_valids(self,

Returns:
Sequence[Tuple[datetime, str]]: A sequence of tuples with valid date/time (indicated by
object's location) and the object's location (path).
object's location) and the object's location (path).
Empty Sequence if no valids found for given time range.
"""
if valid_start and valid_start == valid_end:
Expand Down
86 changes: 86 additions & 0 deletions python/idsse_common/idsse/common/grid_proj.py
Copy link
Contributor

Choose a reason for hiding this comment

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

I found two real comments and a couple nice to haves.

Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
"""Module that wraps pyproj (cartographic/geodetic) library and transforms objects into other data forms"""
# -------------------------------------------------------------------------------
# Created on Mon Jul 31 2023
#
# Copyright (c) 2023 Colorado State University. All rights reserved. (1)
#
# Contributors:
# Mackenzie Grimes (1)
#
# -------------------------------------------------------------------------------
# pylint: disable=invalid-name

from typing import Self, Tuple, Any, Optional

from pyproj import CRS, Transformer
from pyproj.enums import TransformDirection


class GridProj:
"""Wrapper for pyproj instance with methods to convert to and from geographic coordinates, pixels, etc."""
def __init__(self,
crs: CRS,
latitude: float,
longitude: float,
width: float,
height: float,
dx: float,
dy: Optional[float] = None):
# pylint: disable=too-many-arguments,unpacking-non-sequence
self._trans = Transformer.from_crs(crs.geodetic_crs, crs)
self._x_offset, self._y_offset = self._trans.transform(longitude, latitude)
self._w = width
self._h = height
self._dx = dx
self._dy = dy if dy else dx

@classmethod
def from_proj_grid_spec(cls, proj_string: str, grid_string: str) -> Self:
""" Create GridProj instance from projection grid specs

Args:
proj_string (str): pyproj projection string
grid_string (str): Grid string

Returns:
Self: New instance of GridProj which can then be converted to any format
"""
crs = CRS.from_proj4(proj_string)
grid_args = {
key[1:]: float(value)
for key, value in (
pair.split('=') for pair in grid_string.split(' ')
)
}
return GridProj(crs,
grid_args['lat_ll'], grid_args['lon_ll'],
int(grid_args['w']), int(grid_args['h']),
grid_args['dx'], grid_args['dy'])

def map_proj_to_pixel(self, x, y) -> Tuple[float, float]:
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be best of all _to_pixel functions supported rounding, so add the same logic to map_proj_to_pixel() as map_geo_to_pixel() has. It might be good to create a private function to round so that both _to_pixel functions can call the same code.

"""Map projection to a pixel"""
return self.map_geo_to_pixel(*self._trans.transform(x, y)) # pylint: disable=not-an-iterable

def map_pixel_to_proj(self, x: float, y: float) -> Tuple[Any, Any]:
"""Map pixel to a projection"""
return self._trans.transform(*self.map_pixel_to_geo(x, y), direction=TransformDirection.INVERSE)

def map_proj_to_geo(self, x, y) -> Tuple[float, float]:
"""Map projection to geographic coordinates"""
return self._trans.transform(x, y)

def map_pixel_to_geo(self, x: float, y: float) -> Tuple[float, float]:
"""Map pixel to geographic coordinates"""
return x * self._dx + self._x_offset, y * self._dy + self._y_offset

def map_geo_to_proj(self, x: float, y: float) -> Tuple[Any, Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to have: the return type of a tuple of Any is less helpful than Tuple[Union[int, float], Union[int, float]]. I think that is the correct syntax.

"""Map geographic coordinates to a projection"""
return self._trans.transform(x, y, direction=TransformDirection.INVERSE)

def map_geo_to_pixel(self, x: float, y: float) -> Tuple[int, int]:
"""Map geographic coordinates to a pixel

Pixels are only identified by whole numbers when graphically rendered,
so the transformed numerical values (floats) can be safely rounded to ints
"""
return round((x - self._x_offset) / self._dx), round((y - self._y_offset) / self._dy)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Geary-Layne it's safe to assume that pixels can be rounded integers, correct? From my general understanding of how browsers actually paint onto a display, a fraction of a pixel is not possible.

Copy link
Contributor Author

@mackenzie-grimes-noaa mackenzie-grimes-noaa Aug 3, 2023

Choose a reason for hiding this comment

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

I ask because unit testing uncovered this strange precision behavior, before I made the change here to round the resulting pixel. I don't think this extra precision is meaningful (or comprehendible) for a browser client, but I could be wrong:

geo_x, geo_y = self.map_pixel_to_geo(0, 1)
pixel_x, pixel_y = self.map_geo_to_pixel(geo_x, geo_y)

print(pixel_x, pixel_y)  # (0.0 0.9999999999951331)

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct that for some uses, an non integer pixel value is not meaningful and is problematic. There are however some use case for this code (now that it is a general tool and not specific to the slice operator) where you would want floating point numbers. Also, sometime the use would dictate round and sometime floor.

In you test code the precision error is totally executable, and you should us approx().

When I'd implement code like this class in Java and python in the past, I supported an option argument with _to_pixel() function. Something like:
def map_geo_to_pixel(self, x: float, y: float, round: enum=None)
where round can be None, Round, Floor.
Not sure the best way to do this in python, I think I used magic strings before which isn't the best way, and actually making enum seems excessive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel like Enum is overkill here. It makes the comparison easy, although I didn't find an elegant way to directly map, and invoke, Enum members to functions.

Simple if statements seemed sufficient for now, and default to no rounding as previously defined in the data-access-service version of GridProj.

4 changes: 2 additions & 2 deletions python/idsse_common/idsse/common/path_builder.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Module for building (and parsing) paths that are dependent on issue, valid, and lead.

In this weather forecasting data context,
In this weather forecasting data context,
- issue: the datetime when a given forecast was generated (or at least when generation began)
- valid: the datetime when a given forecast "expires" or is superceded by newer forecasts
- lead: the time duration (often in hours) between issue and valid, a sort of forecast "lifespan"
Expand Down Expand Up @@ -49,7 +49,7 @@ def from_dir_filename(cls, dir_fmt: str, filename_fmt: str) -> Self:
"""Construct a PathBuilder object from specified directory and filename formats.
Format string args must follow Python format specifications to define expected fields:
https://docs.python.org/3/library/string.html#format-specification-mini-language

For example, filepaths could be expected to contain "issue" and "lead" fields using format:
'blend.{issue.year:04d}{issue.month:02d}{issue.day:02d}/{issue.hour:02d}/{lead.hour:03d}/'

Expand Down
2 changes: 1 addition & 1 deletion python/idsse_common/idsse/common/publish_confirm.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import pika
from pika.exchange_type import ExchangeType

from idsse.common.log_util import get_default_log_config, set_corr_id_context_var
from idsse.common.log_util import get_default_log_config

logger = logging.getLogger(__name__)

Expand Down
5 changes: 2 additions & 3 deletions python/idsse_common/test/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,8 @@ def __init__(self, config: dict) -> None:
self.b_key = None
super().__init__(config, '', ignore_missing=True)

config = WithoutKeyConfig(
[{'a_key': 'value for a'}, {'b_key': 'value for b'}])

config = WithoutKeyConfig([{'a_key': 'value for a'}, {'b_key': 'value for b'}])

assert config.a_key == 'value for a'
assert config.next.b_key == 'value for b'

Expand Down
133 changes: 133 additions & 0 deletions python/idsse_common/test/test_grid_proj.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
"""Test suite for grid_proj.py"""
# --------------------------------------------------------------------------------
# Created on Wed Aug 2 2023
#
# Copyright (c) 2023 Colorado State University. All rights reserved. (1)
#
# Contributors:
# Mackenzie Grimes (1)
#
# --------------------------------------------------------------------------------
# pylint: disable=missing-function-docstring,redefined-outer-name,invalid-name,protected-access

from typing import Tuple

from pytest import fixture, approx

from idsse.common.grid_proj import GridProj

# example data
EXAMPLE_PROJ_SPEC = '+proj=lcc +lat_0=25.0 +lon_0=-95.0 +lat_1=25.0 +r=6371200'
PROJ_SPEC_WITH_OFFSET = (
'+proj=lcc +lat_0=25.0 +lon_0=-95.0 +lat_1=25.0 +x_0=3271.152832031251 '
'+y_0=263.7934687500001 +r=6371.2, +units=km'
)
EXAMPLE_GRID_SPEC = "+dx=2.539703 +dy=2.539703 +w=2345 +h=1597 +lat_ll=19.229 +lon_ll=-126.2766"

EXAMPLE_PIXELS = [
(0, 0),
(0, 1),
(2000, 1500)
]
EXAMPLE_PROJECTIONS = [
(-126.2766, 19.229),
(-126.27660549554, 19.229022224607),
(-126.23803580508, 19.272773488997)
]
EXAMPLE_GEOS = [
(-3275807.350733, -260554.63043505),
(-3275807.350733, -260552.09073205),
(-3270727.944733, -256745.07593505)
]


# utility to roughly compare tuples of floats with less floating point precision
def approx_tuple(values: Tuple[float, float]) -> Tuple:
return (approx(values[0]), approx(values[1]))


# fixtures
@fixture
def grid_proj() -> GridProj:
return GridProj.from_proj_grid_spec(EXAMPLE_PROJ_SPEC, EXAMPLE_GRID_SPEC)


# test class methods
def test_from_proj_grid_spec(grid_proj: GridProj):
assert isinstance(grid_proj, GridProj)

assert (grid_proj._x_offset, grid_proj._y_offset) == approx_tuple(EXAMPLE_GEOS[0])
assert grid_proj._h == 1597
assert grid_proj._w == 2345
assert grid_proj._dx == approx(2.539703)
assert grid_proj._dy == grid_proj._dx

t = grid_proj._trans
assert t.source_crs is not None and t.source_crs.type_name == 'Geographic 2D CRS'
assert t.target_crs is not None and t.target_crs.type_name == 'Projected CRS'


def test_from_proj_grid_spec_with_offset():
proj_with_offset = GridProj.from_proj_grid_spec(PROJ_SPEC_WITH_OFFSET, EXAMPLE_GRID_SPEC)
assert proj_with_offset._x_offset == approx(-3272536.1979)
assert proj_with_offset._y_offset == approx(-260290.8369)


# transformation methods testing
def test_map_proj_to_pixel(grid_proj: GridProj):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should include testing rounding, once you add rounding option to map_proj_to_pixel().

for index, proj in enumerate(EXAMPLE_PROJECTIONS):
pixel_x, pixel_y = grid_proj.map_proj_to_pixel(*proj)
assert (pixel_x, pixel_y) == EXAMPLE_PIXELS[index]


def test_map_proj_to_geo(grid_proj: GridProj):
for index, proj in enumerate(EXAMPLE_PROJECTIONS):
geo_x, geo_y = grid_proj.map_proj_to_geo(*proj)
assert (geo_x, geo_y) == approx_tuple(EXAMPLE_GEOS[index])


def test_map_pixel_to_geo(grid_proj: GridProj):
for index, pixel in enumerate(EXAMPLE_PIXELS):
geo_x, geo_y = grid_proj.map_pixel_to_geo(*pixel)
assert (geo_x, geo_y) == approx_tuple(EXAMPLE_GEOS[index])


def test_map_pixel_to_proj(grid_proj: GridProj):
for index, pixel in enumerate(EXAMPLE_PIXELS):
proj_x, proj_y = grid_proj.map_pixel_to_proj(*pixel)
assert (proj_x, proj_y) == approx_tuple(EXAMPLE_PROJECTIONS[index])


def test_map_geo_to_proj(grid_proj: GridProj):
for index, geo in enumerate(EXAMPLE_GEOS):
proj_x, proj_y = grid_proj.map_geo_to_proj(*geo)
assert (proj_x, proj_y) == approx_tuple(EXAMPLE_PROJECTIONS[index])


def test_geo_to_pixel(grid_proj: GridProj):
for index, geo in enumerate(EXAMPLE_GEOS):
pixel_x, pixel_y = grid_proj.map_geo_to_pixel(*geo)
assert (pixel_x, pixel_y) == EXAMPLE_PIXELS[index]


def test_compound_tranformations_stay_consistent(grid_proj: GridProj):
# start with pixel, convert to projection
initial_pixel = (EXAMPLE_PIXELS[2][0], EXAMPLE_PIXELS[2][1])
proj_x, proj_y = grid_proj.map_pixel_to_proj(*initial_pixel)
initial_proj = (proj_x, proj_y)

# convert projection to geographic coordinates
geo_x, geo_y = grid_proj.map_proj_to_geo(proj_x, proj_y)
initial_geo = geo_x, geo_y

# convert geographic coordinates back to pixel, full circle, and data should be unchanged
pixel_x, pixel_y = grid_proj.map_geo_to_pixel(geo_x, geo_y)
assert (pixel_x, pixel_y) == initial_pixel

# convert pixel back to geographic coordinates
geo_x, geo_y = grid_proj.map_pixel_to_geo(pixel_x, pixel_y)
assert (geo_x, geo_y) == approx_tuple(initial_geo)

# convert geographic coordinates back to projection
proj_x, proj_y = grid_proj.map_geo_to_proj(geo_x, geo_y)
assert (proj_x, proj_y) == approx_tuple(initial_proj)
Loading