Add doc comments for Dag and minor reformatting. #683
Conversation
| dependenciesOfM: Map[Node[P], List[Node[P]]] = Map[Node[P], List[Node[P]]](), | ||
| dependantsOfM: Map[Node[P], List[Node[P]]] = Map[Node[P], List[Node[P]]]()) { | ||
| /** | ||
| * Summingbird converts a Directed Acyclic Graph(DAG) of Producers into a DAG of Nodes. |
There was a problem hiding this comment.
It's probably worth calling out that Dag objects aren't (typically) constructed directly, but rather through the companion object helpers, which also generate (long) names for each Node.
There was a problem hiding this comment.
Good idea, added that.
| * @param producerToPriorityNames Map every producer to list of names that apply to it. | ||
| * These are the names provided by named nodes. | ||
| * e.g. .name("mynode"). Names flow backwards, i.e from | ||
| * source to tail. |
There was a problem hiding this comment.
Can you clarify the "Names flow backwards, from source to tail" - Actually I think that a Source inherits the name from the Tail
There was a problem hiding this comment.
You're right I got it reversed, fixed.
| @@ -91,20 +91,19 @@ case class SourceNode[P <: Platform[P]](override val members: List[Producer[P, _ | |||
| * @param originalTail The TailProducer before stripping out named nodes. | |||
| * @param producerToPriorityNames Map every producer to list of names that apply to it. | |||
| * These are the names provided by named nodes. | |||
| * e.g. .name("mynode"). Names flow backwards, i.e from | |||
| * source to tail. | |||
| * e.g. .name("mynode"). Names flow from tail to source. | |||
There was a problem hiding this comment.
Can you also call out that the nearest name takes the priority.
There was a problem hiding this comment.
Good idea, done.
| * @param producerToNode What node does a producer map to. Many producers can | ||
| * map into a single node. | ||
| * @param producerToNode Maps producer to node. Many producers can map into a | ||
| * single node. | ||
| * @param nodes All nodes covered by this Dag. | ||
| * @param nodeToName Summingbird assigns a unique name to every node. This | ||
| * is that mapping. Note that this name is different from | ||
| * the name used for applying named options. |
There was a problem hiding this comment.
"This is that mapping" - can you remove it and rephrase.
| * other words, what are the parents of this node. | ||
| * @param dependantsOfM What nodes immediately depend on the given node. In | ||
| * other words what are the children of this node. | ||
| * @param dependenciesOfM Nodes this node immediately depends on. In other words, |
There was a problem hiding this comment.
"Nodes this node immediately depends on" - rephrase
|
seems like a good improvement to style and documentation. you might contrast this to the code in https://github.com/twitter/summingbird/tree/develop/summingbird-core/src/main/scala/com/twitter/summingbird/graph |
|
@johnynek Thanks for pointing out docs for other graph utilities. While reading Dag optimization related code I came across several places where I felt documentation would have been really useful and expected it. I'm trying to add docs to those places, will likely send many more such PRs. Let me know when it feels I'm going overboard :). |
|
👍 |
|
Merged from command line, since I needed to squash commits. |
|
note that github can do squash commits now. After you click the merge On Thu, Aug 18, 2016 at 8:52 AM, Pankaj Gupta notifications@github.com
P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin |
|
Ahh... didn't know that merge button gives squash option. Will do that from next time. |
No description provided.