-
Notifications
You must be signed in to change notification settings - Fork 33
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
Introducing PartialOrder #288
base: main
Are you sure you want to change the base?
Conversation
I thought I'd open a draft pull request to get commentary on choices I've made so far, before I start adding docstrings and tests. First big change is naming. Second is decoupling the way the ids are generated (and their values) from anything in the |
@beverlylytle do you mind short elaborate what the Action class and Plan class is supposed to hold? Is an Action a movement waiting to be path planned? Is it a supposed to hold like a initial config and target config? |
] | ||
|
||
|
||
class IntegerIdGenerator(object): |
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.
Would it make sense to make this thread-safe, like the SequenceCounter
in rrc?
EDIT: https://github.com/compas-rrc/compas_rrc/blob/main/src/compas_rrc/client.py#L25
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.
yep
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.
Is this new version better?
This whole thing is supposed to be very generic. An |
Sorry, I don't fully get it, can you maybe give the intended use example you have in mind for the three classes Action, PlannedAction and Plan. I guess Plan holes an ordered list of PlannedAction? But the use case of Action is not so clear to me. Do I start with a Action. and pass it to a planner somehow, and I will get a PlannedAction in return? And how is the dependency_ids used? Is it like a tree structure for seeking out dependent Actions that are not planned and plan those first? I dont see any information how these classes connect with the planners, so amybe I'm completely guessing wrong. |
Ah, sorry, sure. Yeah, this has nothing to do with any of the planners. This is purely a data structure for holding a plan, and with no algorithmic planning capacity. So, imagine you have three
Now, we need to plan these actions. The idea is that some
But the third brick sits on top of the other two, so for it to be a sensical assembly sequence, they would have to be placed before this third brick. So the third brick depends on the the first two.
Now we have a It would also be possible to traverse this dependency graph (using threading) to complete some tasks held in each action (say, emit some ros messages) in a partially ordered manner. So in the above example, ros messages could be attached to each brick's action for picking and placing that brick. Traversing the graph, messages would be emitted for the picking and placing of bricks 1 and 2, allowing for parallel movement of two robots, and then the ros messages for picking and placing brick 3 wouldn't start until feedback had been received that the tasks for actions 1 and 2 were complete. Does that make more sense @yck011522 ? |
planned_actions : list of :class:`compas_fab.datastructures.PlannedAction` | ||
The planned actions will be stored as an ordered dictionary, so their | ||
ids should be distinct. Defaults to an empty list. | ||
id_generator : Generator[Hashable, None, None] |
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.
Unsure about the typing here, and how sphinx wants it...
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.
I'm leaving some initial feedback, but I'm yet to test this on the project that cannot be named! ;)
return 'Found invalid dependency ids {}'.format(invalid_ids) | ||
|
||
|
||
class Plan(Datastructure): |
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.
Here I have a big question: why not inherit from the compas Graph
instead? Otherwise, we will definitely duplicate features. As an example, I would almost immediately need to be able to turn a plan back and forth from a NetworkX DiGraph, and that works with Graph
but not if the object inherits directly from Datastructure
. The alternative would be containment of a Graph
, but I am not sure that'd be very clean.
self.planned_actions[action_id] = planned_action | ||
return action_id | ||
|
||
def append_action(self, action): |
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.
I find the pair plan_action
and append_action
not very intuitive as being a pair of related functions.
Why not merge them both into add_action
, if the dependency_ids
param is none/empty, then do the appending logic?
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.
I wanted to make a bigger distinction between dependency_ids=None
and dependency_ids=[]
because they result in very different behavior and I can foresee that being the source of annoying bugs for the user.
return dict( | ||
action_id=self.id, | ||
action=self.action, | ||
# sets are not json serializable |
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 message for our future selves... ;)
Co-authored-by: Gonzalo Casas <casas@arch.ethz.ch>
|
||
Returns | ||
------- | ||
:obj:`list` of :obj:`list of :class:`compas_fab.datastructure.Action` |
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.
:obj:`list` of :obj:`list of :class:`compas_fab.datastructure.Action` | |
:obj:`list` of :obj:`list` of :class:`compas_fab.datastructure.Action` |
tests/datastructures/test_plan.py
Outdated
other_plan_data = compas.json_loads(plan_json) | ||
other_plan = Plan.from_data(other_plan_data) | ||
assert plan.actions.keys() == other_plan.actions.keys() | ||
assert other_plan.get_action(1).parameters['param'] == Frame.worldXY() |
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.
@yck011522 Not sure what you mean about Action.from_data
not working... This test passes. What sort of test do you think would fail?
Thanks to your explanation with example. I have a feeling you are trying to do a very specific thing in your project and tries to generalize whatever that is to a bigger class that some others can use it. At the moment I see the only functions for this class are to manage dependency. Which as @gonzalocasas pointed out is pretty much a compas I am guessing your intention for this class is to be able to have a partial plan and allow the computer to reason which things to plan first and what is next. And somehow the first plan changes the state of the scene or the robot such that the next planning depends on the result of previous plan(s). Hence the need for dependency or ordering. If this is the use case, maybe I share a little how I handle this case in my own code. I have a Scene class that keeps track of each object's state (their location as in transformation, their kinematic configuration if it is a RobotModel) it holds the state of the entire scene. I also have an I have a feeling if this is the intention, it would be better to add these Scene and ObjectState related classed and have functions that allow the Actions/Plans and Scene states to interact. And perhaps the function to Path Plan with these Plans and States. I don't want to be discouraging and I appreciate this new PR, but I think more functions that allow these classes to interact with each other (and also with compas_fab classes) would be useful to compas_fab users. |
The purpose of this data structure is to extend Graph to make it easier to create and maintain a graph that is directed and acyclic, representing a partial order on the contents of the nodes of the graph. So this structure is somewhere between a free-for-all graph and a rigid list. Of course everything that is done with this class can be done with Graph. Hell, every class in compas can be represented by a dictionary, but it would be super cumbersome to work always with something as primitive as a dictionary and bag of functions. It seems like the name "Plan" is misleading, as this class has no direct link to path planning. I liked it because it's short, ease to type and suggests something broad and generic (like "list" or "graph"). "DAG" (directed acyclic graph) also hits those notes to some degree, but is less accessible. I am open to suggestions on the name. "Action" is also up for review. It is really just a wrapper around a dictionary. The fact that it is a class and that Python is dynamic provides for some syntactic sugar. But it's not much more than a node in Graph, to the point that I wonder if it even should exist. Since the intention of the class is to be nearly as generic as Graph, I wouldn't expect the Plan to know about Scenes and ObjectStates and managing interactions between them just as Graph doesn't know about Scenes or Frames or Meshes and doesn't manage interactions between them. I think the kind of thing you are suggesting would be well accomplished by applying some variation on the Visitor Pattern to a Graph/Plan. Each user of the Plan could design their own visitors which would allow them to have tailor-made interactions between the contents of the nodes with the contents of other nodes or with external objects like Scenes. This kind of decoupling of data structure from function, and decoupling of data structure from other data structure, allows for much more flexible and reusable code. I am extremely reluctant to add any feature which treats an Action has anything but a container, or which has Plan do anything but manage storing these containers and pointers between them. @gonzalocasas what do you think? |
I'm reading over your comment again @yck011522 and it really sounds to me like you are describing a finite state machine, which is not at all what I'm trying to accomplish with this PR. Or do I misunderstand? |
def check_for_cycles(self): | ||
""""Checks whether cycles exist in the dependency graph.""" | ||
from networkx import find_cycle | ||
from networkx import NetworkXNoCycle | ||
try: | ||
cycle = find_cycle(self.networkx) | ||
except NetworkXNoCycle: | ||
return | ||
raise Exception("Cycle found with edges {}".format(cycle)) | ||
|
||
def get_linear_sort(self): | ||
"""Sorts the planned actions linearly respecting the dependency ids. | ||
|
||
Returns | ||
------- | ||
:obj:`list` of :class:`compas_fab.datastructure.Action` | ||
""" | ||
from networkx import lexicographical_topological_sort | ||
self.check_for_cycles() | ||
return [self.get_action(action_id) for action_id in lexicographical_topological_sort(self.networkx)] |
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.
Do we want these to work in Ironpython?
@beverlylytle I guess I understand it better when you say it's a directed acyclic graph. Maybe the name Plan really gave me high hopes of what the intention of this class wants to do and have me think about automatic planning. Then there are just two things in my mind, regarding the naming I think descriptive names are more important than sounding elegant how about call it DirectedAccylicGraph. Second thing is why is this class in compas_fab but not in compas? I guess if the intention is generic and if there is nothing specific about robotics nor fabrication, then why is it here? I think these two are interelated, can choose to solve either one, it is just that when I look at compas_fab I expect robotic fabrication related classes and functions. And since most people use the package for path planning purposes, I would expect a very high level class with a name called Plan to do what it says. Hold a trajectory or something similar. I wouldn't want to call this DAG an Action too. An action must be actionable to make sense, and your intention seems to be more generic than that. |
Ok, let's see if we can sort this out in a way that satisfies all parties ;) So, the core features of this new class are: 1) to specify "units of stuff that occur", 2) specify their dependencies, 3) extract those "units of stuff" in some order. There are two names that are very closely related but come from different disciplines, ones is the partially ordered set stuff from category theory and the other is partial order plans, and this class is somewhere in between. It lacks the solver parts for automated planning that would make it a better for for the latter option, so, I'm leaning towards Then the other important part is how to expose the traversal of the partial order. The options for traversal are probably very varied. Besides the classic algorithms for traversal, I can think of at least one that is event-based to determine triggering of dependencies (as in the |
Alright, I've renamed it to
While it is important for the use of this data structure, I'm not sure how this comment fits into this PR. I've included two convenience methods for forwarding calls to |
The new name PartialOrder sounds great to me. Clear and descriptive. Perhaps it would be good to keep a link to the Wiki page https://en.wikipedia.org/wiki/Partial-order_planning in the class description. Cheers |
Regarding visitor pattern, is there no generic visitor pattern we can include in the default implementation? |
I'd be happy to add an |
Sounds good to me. I think this abstract method need some documentation for dummies. I am guessing if I want to use this visitor pattern, I need to overwrite the Also, does your implementation always visit each node one time? (Not sure if I Or maybe that is something the user has to deal with. I'm also not sure how the Some other thoughts However, if I anticipate planning can fail due to a bad Child solution, I might want some sort of backtracking ability. I am not knowledgeable enough to say how this can be implemented tho. But maybe there is some elegant way. I think this pattern is very generalizable and often used. |
action can be thought of as pointers to the parents of that planned action. | ||
While actions can be added and removed using the methods of | ||
:attr:`compas_fab.datastructures.PartialOrder.graph`, it is strongly recommended | ||
that the methods ``plan_action``, ``append_action`` and ``remove_action`` |
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.
do you mean get_action
instead of plan_action
?
Another thought is, how about renaming PartialOrder.get_action()
to PartialOrder.action()
? Is there some meaning associated with the get_
prefix?
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.
Actually it should be add_action
not plan_action
.
As for get_action
, the idea is that you provide the key
and get_action
"gets" you the action, that is returns the Action
instance associated to that key
. It's just a typical python thing to name methods with verbs, but some frown on using get_
.
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.
Thanks for clarifying, I just somehow had this idea where if there is a get_
prefix, then I should not modify the returned result in place but rather use the corresponding set_
method. But I could be hallucinating with other language flavours.
Yes, as it's written now, each node is visited exactly one. The I'm sure there's a way to pass a |
This pull request is to introduce the new second-level module
compas_fab.datastructures
and its constituentsPartialOrder
andAction
. These come together to form a partially ordered plan. Closes #202What type of change is this?
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.rst
file in theUnreleased
section under the most fitting heading (e.g.Added
,Changed
,Removed
).invoke test
).invoke lint
).compas_fab.robots.CollisionMesh
.