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

Added doc for strict flag. #2816

Merged
merged 3 commits into from Apr 27, 2015
Merged

Added doc for strict flag. #2816

merged 3 commits into from Apr 27, 2015

Conversation

Thrandis
Copy link
Contributor

Fixes #2651

Is this example enough, or shall I make something with more infos, examples, use cases,...

Cesar

Scan assumes that all the necessary shared variables in ``fn`` are passed as a
part of ``non_sequences``. This has to be ensured by the user. Otherwise, it
will result in an error. This avoids Scan Op calling any earlier (external) Op
over and over, and speeds up the process.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you specify if this cause scan grad speed up, scan optimization speed up or scan execution speed up? All of them?

@memimo
Copy link
Contributor

memimo commented Apr 23, 2015

Format wise, if you compiled it with sphinx locally without any error and warning, then it should be good.
But from user perspective, I couldn't quite understand the use case of the flag and have to go and read the original discussion. Maybe if emphasizing the speed up in the first sentence would make it more clear. And also what happens if I don't set this flag but still pass the shared variables as non sequences?

@Thrandis
Copy link
Contributor Author

Yeah, I agree that it is not really clear. The problem is that there is a lot of different use case. Here are some:

x = T.vector()
W = theano.sared()

# Function with x and W as arguments
def f1(x, W):
    return T.dot(W, x)

# Function without W as argument
def f2(x):
    return T.dot(W, x)

# Here it is ok:
theano.scan(fn=f1, sequences=x, non_sequences=W)
# Here theano complains that f1 takes 2 arguments as input, and only one was provided:
theano.scan(fn=f1, sequences=x)
# Here it is fine:
theano.scan(fn=f2, sequences=x)
# Here it crashes, because strict=True and W was not provided:
theano.scan(fn=f2, sequences=x, strict=True)
# Here it is fine, and the right way to do it:
theano.scan(fn=f1, sequences=x, non_sequences=W, strict=True)

All in all we should say that the best practice is:
1. If you use shared variables inside a function, pass them as argument;
2. When using scan, pass those shared variables as non_sequences, and use strict=True for faster optimization and computation.

This should be the "official" way of using scan. Am I right?
Shall I put something like that in the doc?

@carriepl
Copy link
Contributor

One thing though, it's not using strict=True that makes it faster, it's passing the shared variables as non-sequences. Strict mode only ensures that you did that so it's a convenient way for not forgetting to do it by accident.

@Thrandis
Copy link
Contributor Author

Oh it's a good point actually! I will rephrase the whole thing tomorrow.

@Thrandis
Copy link
Contributor Author

So I changed a bit the way the strict flag is presented: Now it is better integrated with the previous Gibbs smapling example.

@memimo
Copy link
Contributor

memimo commented Apr 24, 2015

Thanks, this is much more clear! If @carriepl approves too, it should be ready for merge.

@carriepl
Copy link
Contributor

I like it. I think it's clear, well explained and it stresses the point that even though you can rely on scan to figure out the shared variables to use, you really ought to pass them as non-sequences yourself.

I don't really have any improvement to suggest. Good work.

memimo added a commit that referenced this pull request Apr 27, 2015
Added doc for strict flag.
@memimo memimo merged commit edbf47e into Theano:master Apr 27, 2015
@Thrandis Thrandis deleted the ccw branch August 20, 2015 12:02
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.

Add example of "strict=True" in documentation of scan
4 participants