-
Notifications
You must be signed in to change notification settings - Fork 6
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: add generation of random points in grid_plan #132
Conversation
suggestions
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
- Coverage 98.19% 98.18% -0.02%
==========================================
Files 14 14
Lines 833 880 +47
==========================================
+ Hits 818 864 +46
- Misses 15 16 +1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a general thought here, (wasn't a good specific place to comment inline in the code)
I love that you're exploring iterators here! but i think in this case it's gotten a bit more complicated than it needs to be. here's a few thoughts in no particular order:
- note that a numpy array is already an iterable, so returning
iter(seed.uniform(0, 1, size=size))
doesn't actually gain anything over just returningseed.uniform(0, 1, size=size)
(but it is a bit harder to reason about perhaps?) - as written,
MAX_ITERS
could more accurately be calledMIN_RANDOM_POINTS
, because when you calliter_size = max(num_points, MAX_ITER)
, you're guaranteeing that the number of random generated points will be at least as bit asMAX_ITER
, (unlessnum_points
is greater). That's fine in this case, cause numpy is so fast... but the naming might be a bit surprising (even though the number of points does set an upper bound of the maximum possible number of iterations) - doing stuff like
math.sqrt(x0) * (max_width / 2) * math.cos(angle * 2 * math.pi)
is going to be slower in a python for loop than it would be if you simply greedily calculated many thousands of candidate points in numpy and iterated over them. - What began as a relatively simple pattern (
[p for point in random_points() if _is_a_valid_point(p)] has gotten complicated and passed more parameters through to the deepest functions
_is_a_valid_pointthan necessary, and the
PointGenerator` callable type is now very hard to look at and reason about:PointGenerator = Callable[ [int, float, float, Tuple[Optional[float], Optional[float]], bool, Optional[int]], Iterable[Tuple[float, float]], ]
Note that all shape types share the same _is_a_valid_point
function... so I think it would be better not to pass min_distance
and allow_overlap
through all of those functions... instead just generate your 5000 points up front, and then iterate through those in the main iter function:
so, I'm thinking something like this:
class RandomPoints(_PointsPlan):
def __iter__(self) -> Iterator[GridPosition]: # type: ignore
seed = np.random.RandomState(self.random_seed)
func = _POINTS_GENERATORS[self.shape]
points: list[Tuple[float, float]] = []
for x, y in func(seed, self.max_width, self.max_height):
if (
self.allow_overlap
or (None in (self.fov_width, self.fov_height))
or _check_validity(self.fov_width, self.fov_height, points, x, y)
):
yield GridPosition(x, y, 0, 0, True)
points.append((x, y))
if len(points) >= self.num_points:
break
where:
PointGenerator = Callable[[np.random.RandomState, float, float], np.ndarray]
_POINTS_GENERATORS: dict[Shape, PointGenerator]
def _is_a_valid_point(
points: list[Tuple[float, float]],
x: float,
y: float,
min_dist_x: float,
min_dist_y: float,
) -> bool:
return not any(
abs(x - point_x) < min_dist_x or abs(y - point_y) < min_dist_y
for point_x, point_y in points
)
and _random_points_in_ellipse
and _random_points_in_rectangle
just greedily make 5000 candidate points and return a numpy array
Co-authored-by: Talley Lambert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! All set for merge?
yes! Thanks for the help! |
This PR adds the
RandomPoints
plan, a type ofgrid_plan
with the ability to generate a specified number of random points within a specified area.The random generation is using
numpy
and if arandom_seed
attribute is specified, the generation of points is reproducible.By setting
allow_overlap=True
together withfov_width
andfov_height
, the generated point will be at leastfov_width
andfov_height
away from each other (Manhattan distance).For quick test to see how it works: