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

Suggested edit to message passing notation in junction tree notes #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apappu97
Copy link

I believe in line with the notation earlier in the notes and discussion of message passing, the sentence "The factor $$\tau_j(x_j)$$ can be thought of as a message that ..." should instead be "The factor $$\tau_{jk}(x_k)$$ ..." as we've summed over the values of variable $$x_j$$, leaving a new factor that is a function of $x_k$, i.e. the message from $$x_j$$ to $$x_k$$.

@apappu97 apappu97 changed the title Small suggested edit to message passing notation Suggested edit to message passing notation in junction tree notes May 22, 2020
chrisyeh96 added a commit to chrisyeh96/cs228-notes that referenced this pull request May 23, 2020
Responds to Pull Request ermongroup#198 by reverting a change made in commit feca805. Additionally fixes some typos.
@chrisyeh96
Copy link
Collaborator

"The factor $$\tau_j(x_j)$$" is actually correct as written. $$\tau_j(x_j)$$ indeed summarizes all of the information from the subtree rooted at $$x_j$$. I have made this notation more consistent by rewriting $$\tau_{jk}(x_k)$$ as $$\tau_k(x_k)$$.

@chrisyeh96 chrisyeh96 closed this May 23, 2020
@apappu97
Copy link
Author

Oh okay, thanks for clarifying. And thanks for all of the effort on these notes, they are wonderful!

To followup: isn't the subscript redundant then? As a reader, and having cross referenced with other message passing notes, the double index subscript feels much clearer, as it makes explicit the destination and source nodes in the message passing algorithm.

Additionally, the single index subscript notation seems inconsistent with the explicit $$m_{i -> j}$$ notation used later on once the $$\tau$$ notation is replaced by messages, as foreshadowed in Line 21 (copying from your edit):

"The factor $$\tau_j(x_j)$$ can be thought of as a message that $$x_j$$ sends to $$x_k$$ that summarizes all of the information from the subtree rooted at $$x_j$$."
---> this is a bit unclear as it doesn't have explicit $$x_k$$ dependence, and showing the dependence on the 'sender' node $$x_j$$ isn't possible without a double subscript.

Also on line 21: "At each step, we will eliminate $$x_j$$; this will involve computing the factor $$\tau_k(x_k) = \sum_{x_j} \phi(x_k, x_j) \tau_j(x_j)$$, where $$x_k$$ is the parent of $$x_j$$ in the tree."
---> If $$\tau_k(x_k)$$ indeed is all of the information from the subtree rooted at $$x_k$$, the notation as used in this line precludes there existing multiple children of node $$x_k$$. I think this would also be clearer if the original notation of $$\tau_{jk}$$ was kept to make explicit the child-parent/sender-receiver relationship.

If you agree, I'm happy to make changes. Thanks for looking!

chrisyeh96 added a commit to chrisyeh96/cs228-notes that referenced this pull request May 23, 2020
Responds to Pull Request ermongroup#198 by reverting a change made in commit feca805. Additionally fixes some typos.
@chrisyeh96
Copy link
Collaborator

Thanks for taking the time to read these notes so carefully! Your point about potentially having multiple children is something I overlooked, so I'm going to re-open this pull request. Will come back and think through this more thoroughly. I think there is significant room to improve this section of the notes. Just a small example: the notes on VE never mention "forming cliques" of a certain size and instead talks about the size of factor scopes. I'll take a deeper pass through this when I have time. Feel free to continue making changes as you see fit and I'll see what makes sense to incorporate!

@chrisyeh96 chrisyeh96 reopened this May 23, 2020
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