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

Bug fix to vectaster #60

Merged
merged 10 commits into from
Apr 19, 2024
Merged

Bug fix to vectaster #60

merged 10 commits into from
Apr 19, 2024

Conversation

Geary-Layne
Copy link
Contributor

Linear Issue

IDSSE-638

Changes

  • The two bugs
    • Not including edge pixels when rasterizing polygons
      • Now after getting 'fill' pixels, also get edge pixels
    • Sometimes pixels were identified more than once when rasterizing polygons
      • Now uses sets
  • Other changes
    • Added the ability to create a geo image without background data, helpful for testing
    • Added functionality to ColorPalettes to help scaling color range, helpful for WindGust color palette
    • Added support for passing geojson object as dictionary to some functions
    • Increased test coverage

Explanation

N/A

@Geary-Layne Geary-Layne self-assigned this Apr 17, 2024
Copy link
Contributor

@mackenzie-grimes-noaa mackenzie-grimes-noaa left a comment

Choose a reason for hiding this comment

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

No changes needed, just comments to be sure I understand what's happening in this code.

@@ -368,9 +374,13 @@ def _pixels_for_linestring(
list[Pixel]: List of x,y tuples in pixel space
"""
coords = linestring.coords
pixels = _pixels_for_line_seg(coords[0], coords[1])
pixels = set(_pixels_for_line_seg(coords[0], coords[1]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check for understanding: it looks like we turn the pixel list into a set to prevent duplicates here, but the _pixels_for_line_seg() function we called also turns them into a set before returning them to us.

Is there a reason that _pixels_for_line_seg() isn't guaranteed to return unique pixels, but the similar function _pixels_for_polygon() is? Maybe because Polygons have boundaries + edges so the logic in that function is more complex and complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting set around _pixels_for_line_seg() isn't to insure the returned pixels are unique (they are, as are the returned list from three pixels_for...), it is there to cast the returned list to a set, so that when additional pixels are added, they are assured to be unique (notice the pixels.update() below). It might be better to have the return type of all three of these be sets rather than lists, but I'm not sure if there would be any impacts from returning collections, vs sequence. I'll look into this on my next task

Self: GeoImage
"""
rgb_array = np.zeros((proj.width*scale, proj.height*scale, 3), np.uint8)
rgb_array[...] = fill_color
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never seen this syntax in Python, what does this do? Is it shorthand for looping through all the values in the array and setting them to fill_color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's call the Ellipsis object, I found it in some example code. I originally thought it was just part of numpy but after you comment I looked it up and it is a python thing. In this case it is equivalent to rgb_array[:,:] = fill_color, which sets the third dim equal to fill_color for all of the first 2 dims. So for each pixel location (i,j) set the color to fill_color.

@Geary-Layne Geary-Layne merged commit 9460fad into main Apr 19, 2024
2 checks passed
@Geary-Layne Geary-Layne deleted the bug/idsse-638/test-syracuse branch April 19, 2024 19:56
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.

2 participants