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

Filter by unbound application and call_id #1194

Merged
merged 1 commit into from
Jul 13, 2017

Conversation

rizar
Copy link
Contributor

@rizar rizar commented Jul 7, 2017

This PR does two things:

  1. It makes it possible to filter by an unbound application, e.g. Linear.apply
  2. It introduces an optional keyword argument call_id that writes a call identifier to the metadata attribute of the ApplicationCall object. VariableFilter is modified to support filtering by this new attribute. The usecase that I have in mind is when an application method is called twice, e.g. an RNN is used to encode both the question the answer. It is useful in such cases to make a distinction between the variables created in the first and in the second call.

@dmitriy-serdyuk
Copy link
Contributor

I have a question about 2. Does the call id propagate recursively to applications inside the top level application? Do you plan to annotate every brick with the call id?

It seems that this feature needs to be documented. Maybe in some future PR.

@rizar
Copy link
Contributor Author

rizar commented Jul 12, 2017

Does the call id propagate recursively to applications inside the top level application?

Nope.

Do you plan to annotate every brick with the call id?

Nope, call id can be passed like brick.apply(x,y,z, call_id='123') and it will be assigned to application call not to a brick. As for documenting it, it is not very clear where it should be documented: as_dict and as_list, which are implemented in a very similar way, aren't documented.

@rizar
Copy link
Contributor Author

rizar commented Jul 13, 2017

So, what's your opinion, @dmitriy-serdyuk ?

@dmitriy-serdyuk
Copy link
Contributor

It looks good. But I don't understand why the test fails.

@rizar
Copy link
Contributor Author

rizar commented Jul 13, 2017 via email

@rizar rizar merged commit 979d0f9 into mila-iqia:master Jul 13, 2017
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