feat: IDSSE-256: GridProj #5

mackenzie-grimes-noaa · 2023-08-02T23:47:57Z

Linear Task

Changes

Move GridProj class from data-access-service to this project
- Added optional rounding parameter for transforming other data formats into pixel ints. By default, return pixel values as floats.
Write unit tests for existing functionality: transforming between 3 data types:
- Pixel
- Projection
- Geographic Coordinates
Cleanup any flake8 linter warnings

paulhamer-noaa

Looks reasonable.

Geary-Layne

LGTM
The only thing I wonder about is the mix of EXAMPLE data values defined at the top of unit test and literal values found in many test functions. To me there are two styles and I don't see the distinction. Not that the code needs to change, I was just wondering the reason.

mackenzie-grimes-noaa · 2023-08-03T18:51:06Z

python/idsse_common/idsse/common/grid_proj.py

+        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)


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

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)

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.

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.

…ng (none by default)

mackenzie-grimes-noaa · 2023-08-04T20:37:53Z

python/idsse_common/idsse/common/grid_proj.py

+            return (round(pixel_x), round(pixel_y))
+        if rounding is PixelRounding.FLOOR:
+            return (floor(pixel_x), floor(pixel_y))
+        return (pixel_x, pixel_y)


@Geary-Layne I added a rounding argument to map_geo_to_pixel that is either ROUND, FLOOR or None, is this about what you envisioned?

Geary-Layne

Biggest thing is the need to update the rounding logic. Also make sure that the result from rounding are ints vs float that represent int values.

Geary-Layne · 2023-08-09T16:36:18Z

python/idsse_common/idsse/common/grid_proj.py

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

Geary-Layne · 2023-08-09T16:37:45Z

python/idsse_common/idsse/common/grid_proj.py

+from pyproj.enums import TransformDirection
+
+
+class PixelRounding(Enum):


Nice to have: could we make this so the caller to _to_pixel could use strings, say 'ROUND', 'round', 'FLOOR', or 'floor'?

Geary-Layne · 2023-08-09T16:39:19Z

python/idsse_common/idsse/common/grid_proj.py

+                        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]:


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.

Geary-Layne · 2023-08-09T16:40:58Z

python/idsse_common/idsse/common/grid_proj.py

+        """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]:


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.

Geary-Layne · 2023-08-09T16:45:02Z

python/idsse_common/idsse/common/grid_proj.py

+        pixel_y = (y - self._y_offset) / self._dy
+
+        if rounding is PixelRounding.ROUND:
+            return (round(pixel_x), round(pixel_y))


Python 3.x switch rounding logic from 2.x and it is not what we want to use here. In 3.x the rounding logic is "round half to even" and it is better for our use to "round half away from zero". In place of round() use "return floor(x + 0.5) if x >= 0.0 else ceil(x - 0.5)". This works since we are always round to the ones place, it is more complex if we needed to round to a specified diget.

Ah, didn't know our requirements were rounding away from zero instead of to the nearest even. From my reading, numpy.round() behaves the same as Python 3.x round().

IEEE-754: floating-point rounding

Do we already have a utility function for this rounding technique? Otherwise the one we're making here should probably be used across IDSS projects for consistency.

I don't believe we have a utility function for rounding. It would be straight forward to add a "round half away from zero, to the ones place" to utils.py. It would be better to implement to round to an optional decimal place. If you would like to do this (either one) and have GridProj call it, that would be great.

Great, doing that now. Created a sub-task in linear to track this (IDSSE-306).

Geary-Layne · 2023-08-09T16:48:41Z

python/idsse_common/idsse/common/grid_proj.py

+            Tuple[Union[int, float], Union[int, float]):
+                x, y values for pixel, rounded to ints if rounding parameter passed, otherwise floats
+        """
+        pixel_x = (x - self._x_offset) / self._dx


This is probably just my preference so feel free to ignore. I like to use i,j for pixel space vs x,y.

Geary-Layne · 2023-08-09T16:50:05Z

python/idsse_common/test/test_grid_proj.py

+
+    # 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 (round(pixel_x, 6), round(pixel_y, 6)) == initial_pixel


This will have to change when you update the round logic in grid_proj.

Should the test now be using round_half_away, or is round correct?

This was still using the incorrect round function, good catch. Fixed now.

Geary-Layne · 2023-08-09T16:50:44Z

python/idsse_common/test/test_grid_proj.py

+
+
+# transformation methods testing
+def test_map_proj_to_pixel(grid_proj: GridProj):


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

…proj

mackenzie-grimes-noaa · 2023-08-11T23:29:48Z

This PR is now blocked by #7 which contains the rounding utility function we need here.

…proj

mackenzie-grimes-noaa · 2023-08-18T01:51:28Z

@Geary-Layne I remembered now that in the GridProj class, I chose not to call my custom rounding function from the 2nd pixel-converting method --map_proj_to_pixel()-- because I discovered that it simply calls the 1st pixel method --map_geo_to_pixel()-- which in turn calls the rounding function.

So map_proj_to_pixel() would just be rounding an already-rounded float. Instead it passes the rounding arguments (rounding and precision) down to map_geo_to_pixel() and lets it do the rounding.

I can refactor that to be less confusing if you think of a better way.

Geary-Layne

Rounding function looks good.
It makes me happy to see our test code being build out.

Geary-Layne · 2023-08-18T14:40:15Z

python/idsse_common/idsse/common/utils.py

@@ -83,8 +84,7 @@ def exec_cmd(commands: Sequence[str], timeout: Optional[int] = None) -> Sequence
        Sequence[str]: Result of executing the commands
    """
    logger.debug('Making system call %s', commands)
-    # with Popen(commands, stdout=PIPE, stderr=PIPE) as proc:
-    #     out = proc.readlines()
+    # pylint: disable=consider-using-with


Why did you decide to not use "with Popen"? In the current implementation is process ever closed?

I guess I assumed it was commented out for a reason and simply silenced the warning. We can re-enable those lines.

It is not clear to me the correct way to use subprocess. Maybe proc.kill() actually frees underlying resources, but if that is the case there is a few line lower that I'm not sure will always work. I think it good enough for now, but maybe we should create a task to figure out the correct to leverage subprocess.

Geary-Layne · 2023-08-18T14:43:17Z

python/idsse_common/test/test_grid_proj.py

+
+    # 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 (round(pixel_x, 6), round(pixel_y, 6)) == initial_pixel


Should the test now be using round_half_away, or is round correct?

mackenzie-grimes-noaa added 2 commits August 2, 2023 17:36

create grid_proj and unit tests

b2c059b

cleanup flake8 warnings

6fdc41c

mackenzie-grimes-noaa marked this pull request as ready for review August 2, 2023 23:49

mackenzie-grimes-noaa requested review from Geary-Layne and paulhamer-noaa August 2, 2023 23:49

paulhamer-noaa approved these changes Aug 3, 2023

View reviewed changes

Geary-Layne approved these changes Aug 3, 2023

View reviewed changes

cleanup scattered literal numbers in test_grid_proj, round pixels to int

58bbd45

mackenzie-grimes-noaa commented Aug 3, 2023

View reviewed changes

mackenzie-grimes-noaa added 2 commits August 3, 2023 16:02

add optional rounding param to map_geo_to_pixel which controls roundi…

5d216eb

…ng (none by default)

fix docstring in map_geo_to_pixel

c60d13b

mackenzie-grimes-noaa requested a review from Geary-Layne August 4, 2023 20:36

mackenzie-grimes-noaa commented Aug 4, 2023

View reviewed changes

Geary-Layne requested changes Aug 9, 2023

View reviewed changes

mackenzie-grimes-noaa added 6 commits August 11, 2023 14:39

add round_half_up() utility function, unit tests

89d7611

add tests for Map class, exec_cmd, dict_copy_with

9a4c6eb

add unit test for Map __setattr__

e6776d9

Merge branch 'feat/idsse-306/round-half-up' into feat/idsse-246/grid-…

6b7350f

…proj

add _round_pixel_maybe to grid_proj to standardize pixel rounding

e7c7be4

fix grid_proj unit tests

9578861

mackenzie-grimes-noaa added the 🚧 DO_NOT_MERGE 🚧 PR in progress or blocked by outside forces label Aug 11, 2023

mackenzie-grimes-noaa and others added 8 commits August 15, 2023 14:27

round_half_away() now uses only math ceil/floor, no Decimal

8aa1685

Merge branch 'main' into feat/idsse-306/round-half-up

1dd8a63

cleanup pylint warnings that are my fault

252f9c9

Merge branch 'feat/idsse-306/round-half-up' into feat/idsse-246/grid-…

8c574f0

…proj

fix rounding tests in test_grid_proj, pylitn warnings

d60c7ce

more specific type hints in grid_prjo

9de285e

make _round_pixel_maybe a static method

63618b0

remove unused imports

ce4c78d

Geary-Layne approved these changes Aug 18, 2023

View reviewed changes

fix all test_grid_proj rounding to use correct round function

b287da4

Geary-Layne approved these changes Aug 21, 2023

View reviewed changes

mackenzie-grimes-noaa removed the 🚧 DO_NOT_MERGE 🚧 PR in progress or blocked by outside forces label Aug 21, 2023

mackenzie-grimes-noaa merged commit 4bb8e47 into main Aug 21, 2023
2 of 3 checks passed

mackenzie-grimes-noaa mentioned this pull request Aug 21, 2023

[feat] IDSSE-306: round_half_away utility #7

Closed

mackenzie-grimes-noaa deleted the feat/idsse-246/grid-proj branch September 18, 2023 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: IDSSE-256: GridProj #5

feat: IDSSE-256: GridProj #5

mackenzie-grimes-noaa commented Aug 2, 2023 •

edited

Loading

paulhamer-noaa left a comment

Geary-Layne left a comment

mackenzie-grimes-noaa Aug 3, 2023

mackenzie-grimes-noaa Aug 3, 2023 •

edited

Loading

Geary-Layne Aug 3, 2023

mackenzie-grimes-noaa Aug 3, 2023

mackenzie-grimes-noaa Aug 4, 2023

Geary-Layne left a comment

Geary-Layne Aug 9, 2023

Geary-Layne Aug 9, 2023

Geary-Layne Aug 9, 2023

Geary-Layne Aug 9, 2023

Geary-Layne Aug 9, 2023

mackenzie-grimes-noaa Aug 9, 2023

Geary-Layne Aug 9, 2023

mackenzie-grimes-noaa Aug 9, 2023

Geary-Layne Aug 9, 2023

Geary-Layne Aug 9, 2023

Geary-Layne Aug 18, 2023

mackenzie-grimes-noaa Aug 18, 2023

Geary-Layne Aug 9, 2023

mackenzie-grimes-noaa commented Aug 11, 2023 •

edited

Loading

mackenzie-grimes-noaa commented Aug 18, 2023

Geary-Layne left a comment

Geary-Layne Aug 18, 2023

mackenzie-grimes-noaa Aug 18, 2023

Geary-Layne Aug 18, 2023

Geary-Layne Aug 18, 2023

		from pyproj.enums import TransformDirection


		class PixelRounding(Enum):



		# transformation methods testing
		def test_map_proj_to_pixel(grid_proj: GridProj):

feat: IDSSE-256: GridProj #5

feat: IDSSE-256: GridProj #5

Conversation

mackenzie-grimes-noaa commented Aug 2, 2023 • edited Loading

Linear Task

Changes

paulhamer-noaa left a comment

Choose a reason for hiding this comment

Geary-Layne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mackenzie-grimes-noaa Aug 3, 2023 • 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

Geary-Layne left a 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

Choose a reason for hiding this comment

mackenzie-grimes-noaa commented Aug 11, 2023 • edited Loading

mackenzie-grimes-noaa commented Aug 18, 2023

Geary-Layne left a 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

mackenzie-grimes-noaa commented Aug 2, 2023 •

edited

Loading

mackenzie-grimes-noaa Aug 3, 2023 •

edited

Loading

mackenzie-grimes-noaa commented Aug 11, 2023 •

edited

Loading