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

Made merge brick apply application #619

Merged
merged 7 commits into from
May 5, 2015

Conversation

dmitriy-serdyuk
Copy link
Contributor

No description provided.

@dmitriy-serdyuk
Copy link
Contributor Author

There is something wrong with Merge brick. This code

z = merge.apply(x, y)
cg = ComputationGraph(z)
VariableFilter(bricks=[merge], roles=[OUTPUT])(cg.variables)

returns [merge_apply_x, merge_apply_y].

@dmitriy-serdyuk
Copy link
Contributor Author

I added a test for this case. Do you have any ideas why is is happening?

@rizar
Copy link
Contributor

rizar commented May 5, 2015

I think this weird behavior is due to the fact that Merge inherits from Parallel and call the parent's apply method in its apply method. However, I would then expect three OUTPUT variables: two of Parallel.apply and one of Merge.apply.

@rizar
Copy link
Contributor

rizar commented May 5, 2015

As for the main body of your PR, it looks all right to me.

@dmitriy-serdyuk
Copy link
Contributor Author

Yes, you are right, there were 3 outputs.

@rizar
Copy link
Contributor

rizar commented May 5, 2015

But then it is completely expected behavior, isn't it? Should your new test be fixed to check that 3 variables are found?

@dmitriy-serdyuk
Copy link
Contributor Author

It is still returning all three variables. I don't think that it is expected. Why filtering by applications gives only one output, but filtering by bricks returns outputs of all predecessors?

@rizar
Copy link
Contributor

rizar commented May 5, 2015

Because when you call merge.apply it calls super(Merge, self).apply, that is two different application methods of the same brick merge are called. That's why what you get when you filter outputs of the merge brick are outputs of both application methods, two of Parallel.apply and one of Merge.apply.

@dmitriy-serdyuk
Copy link
Contributor Author

Ok, I see. But it doesn't seem intuitive.

@rizar
Copy link
Contributor

rizar commented May 5, 2015

That is an inevitable consequence of having Merge as a descendant of Parallel. Whether or not this was a good design decision can be discussed.

rizar added a commit that referenced this pull request May 5, 2015
Made merge brick apply application
@rizar rizar merged commit 27cb826 into mila-iqia:master May 5, 2015
@rizar
Copy link
Contributor

rizar commented May 5, 2015

Thank for this small clean-up anyway :)

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