Skip to content

Conversation

rdevon
Copy link
Contributor

@rdevon rdevon commented Oct 22, 2014

Added RBM subclass of DBM for convenience.
Add docs to DBM.
Added chain initialization to DBM for some future changes to gradient estimation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doc is more user-facing. The comment about which inference procedure would make more sense as a comment starting with '#' in the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm

@rdevon
Copy link
Contributor Author

rdevon commented Oct 22, 2014

OK, the tests are failing here but passing locally. I'll figure this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is completely accurate, as it depends on the assumption from which you're basing your MF parameterization, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

The parameterization can make the MF update more expensive but it can't make you need more than one update. This is assuming the model really is an RBM.

@rdevon
Copy link
Contributor Author

rdevon commented Oct 23, 2014

Ian could you take a look at the Travis build error... it's not happening locally, and honestly I don't know why bvmp tests are failing: this PR has nothing to do with the methods or classes implemented there.

@rdevon
Copy link
Contributor Author

rdevon commented Oct 23, 2014

Question about DBM implementation in general: currently if you're training a generate model with the labels, the modeling layer is the last hidden layer of the DBM. However, this seems to add extra switches throughout to account for this. Wouldn't it be better to implement the label layer as another special layer (like visible_layer), or to even perhaps make the visible layers a list of layers? What if you had other observables as well?

@lamblin
Copy link
Member

lamblin commented Oct 23, 2014

@rdevon These errors seem to have been introduced by a recent change in Theano, see Theano/Theano#2195.

@lamblin
Copy link
Member

lamblin commented Oct 23, 2014

I restarted travis, the problem should have been fixed in Theano.

@rdevon
Copy link
Contributor Author

rdevon commented Oct 24, 2014

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't accurate. Exact inference over the hidden units given all the visible units of an RBM doesn't require any kind of sampling. In that context, exact inference is equivalent to mean field inference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, yes. My bad.

@goodfeli
Copy link
Contributor

goodfeli commented Nov 6, 2014

This needs a rebase now. When you rebase, go ahead and squash so we don't have too many separate commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

This returns None, should return a bool

Added RBM subclass of DBM for convenience.
Add docs to DBM.
Added chain initialization to DBM for some future changes to gradient estimation.

Starting PR to push DBN to Pylearn2.

Added RBM subclass of DBM for convenience.
Add docs to DBM.
Added chain initialization to DBM for some future changes to gradient estimation.

fixed line formatting

Made the docs numpydoc compliant.
Modified inference procedure to have a method "is_rbm_compatible" that will raise a NotImplementedError if not appropriate for RBM.
Removed assert for RBM and UpDown inference.

Made the docs numpydoc compliant.
Modified inference procedure to have a method "is_rbm_compatible" that will raise a NotImplementedError if not appropriate for RBM.
Removed assert for RBM and UpDown inference.
@rdevon
Copy link
Contributor Author

rdevon commented Nov 7, 2014

OK, rebased and made changes. I think this will be the last PR to this branch, as I'm moving development to dbm_v2... dbn will follow it. After merge I will bring these changes to dbm_v2.

@dwf
Copy link
Contributor

dwf commented Feb 11, 2015

@goodfeli You were reviewing this originally, any update?

@rdevon
Copy link
Contributor Author

rdevon commented Feb 11, 2015

Got side-tracked. Hoping to pick this back up eventually but don't have time ATM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants