Add routing when suggesting multiple points #217
Conversation
thopkins32
left a comment
thopkins32
left a comment
There was a problem hiding this comment.
Looks good so far. I made a few comments/suggestions.
Also needed before merge:
- Unit tests
- Discrete suggestions
- Validate
"_id"keys are consistent after the reordering
- API reference docs
| if route and n_points > 1: | ||
| suggestions = route_suggestions(suggestions) |
There was a problem hiding this comment.
If we can read the current positions, we can include that in the graph for the optimal routing, otherwise, we can fallback to routing the rest.
| if route and n_points > 1: | |
| suggestions = route_suggestions(suggestions) | |
| if route and n_points > 1: | |
| if all(isinstance(actuator, Readable) for actuator in actuators): | |
| current_pos = yield from read(cast(Sequence[Readable], actuators)) | |
| else: | |
| current_pos = None | |
| suggestions = route_suggestions(suggestions, starting_point=current_pos) |
| for j, j_point in enumerate(points): | ||
| if i >= j: | ||
| continue | ||
| G.add_weighted_edges_from([(i, j, np.sqrt(np.sum(np.square(i_point - j_point))))]) |
There was a problem hiding this comment.
This part assumes all suggestions are floats when they can also be discrete choices (like strings). Need to add filtering so we only route based on distances we can compute, along with unit test coverage for the variety of cases.
| return np.array(nx.approximation.traveling_salesman_problem(G, cycle=cycle)) | ||
|
|
||
|
|
||
| def route_suggestions(suggestions: list[dict], cycle: bool = False): |
There was a problem hiding this comment.
Not sure we will ever need to return to the first point.
| return np.array(nx.approximation.traveling_salesman_problem(G, cycle=cycle)) | |
| def route_suggestions(suggestions: list[dict], cycle: bool = False): | |
| return np.array(nx.approximation.traveling_salesman_problem(G, cycle=False)) | |
| def route_suggestions(suggestions: list[dict]): |
| if route and n_points > 1: | ||
| suggestions = route_suggestions(suggestions) | ||
|
|
||
| uid = yield from acquisition_plan(suggestions, actuators, optimization_problem.sensors, *args, **kwargs) |
There was a problem hiding this comment.
I think it makes sense to have this in default_acquire rather than optimize_step.
I know that in the future when sending plans to the queueserver, you will have to use default_acquire over optimize_step, since the optimizer class is not serializable. We will want the routing available in the queueserver.
It also makes it really easy to opt-out of any routing by bringing your own custom acquisition plan.
|
|
||
|
|
||
| def route_suggestions(suggestions: list[dict], cycle: bool = False): | ||
| dims = [dim for dim in suggestions[0] if dim != "_id"] |
There was a problem hiding this comment.
Can import the ID_KEY and use it here
| dims = [dim for dim in suggestions[0] if dim != "_id"] | |
| dims = [dim for dim in suggestions[0] if dim != ID_KEY] |
2 participants
And remove outdated TSP dependencies