Skip to content
This repository has been archived by the owner on Sep 1, 2023. It is now read-only.

Added overlap accessors to spatial_pooler.py #3153

Merged
merged 6 commits into from
Jun 2, 2016

Conversation

hernandezurbina
Copy link

@hernandezurbina hernandezurbina commented Jun 1, 2016

Fixes #3148

@numenta-ci
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @scottpurdy, @subutai and @chetan51 to be potential reviewers

@rhyolight
Copy link
Member

@hernandezurbina Hey Victor, please have a quick read: https://github.com/numenta/nupic/wiki/Development-Process

You must create a public issue describing the work that needs to be done. Then link to it from this PR description with fixes #XXX.

@subutai
Copy link
Member

subutai commented Jun 1, 2016

@hernandezurbina There is already an existing issue that you should link to:
#3148

@subutai
Copy link
Member

subutai commented Jun 1, 2016

@hernandezurbina Please see the details of the #3148 - you need to handle backward compatibility with pickling. @mrcslws might be able to help you with the details.

@hernandezurbina hernandezurbina changed the title Add overlap accessors to spatial_pooler.py #3148 Added overlap accessors to spatial_pooler.py Jun 1, 2016
@hernandezurbina
Copy link
Author

@rhyolight @subutai @mrcslws I think it's done. Could you please have a look? Cheers!

def getOverlaps(self):
"""Returns the overlap score for each column."""
return self._overlaps

Copy link
Member

Choose a reason for hiding this comment

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

Two blank lines between methods.

@subutai
Copy link
Member

subutai commented Jun 1, 2016

@hernandezurbina Looks good. Left a few minor comments. Please read through our Python style guide:
https://github.com/numenta/nupic/wiki/Python-Style-Guide

if state['_version'] < 3:
# the overlaps and boostedOverlaps properties were added in version 3,
state['_overlaps'] = numpy.zeros(self._numColumns)
state['_boostedOverlaps'] = numpy.zeros(self._numColumns)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably also set the dtypes here to be consistent.

@hernandezurbina
Copy link
Author

@subutai OK, it's fixed now. Now I'm reading the Python style guide.

@mrcslws
Copy link
Contributor

mrcslws commented Jun 1, 2016

Really, we don't need to initialize these numpy arrays. We could just set self._overlaps and self._boostedOverlaps to None. We build a new array every timestep, so these arrays are never actually used. If we allocate an array in the __init__ method, it implies that we're going to reuse it.

This would save us from having numpy.zeros calls all over the place. Whether we reuse the arrays or not, there only needs to be one code location that calls numpy.zeros (or rather, one for each of the two arrays).

@hernandezurbina
Copy link
Author

@subutai @mrcslws OK, I added the dtype specifications, and left the numpy.zeros calls in init as discussed.

@subutai
Copy link
Member

subutai commented Jun 1, 2016

@rhyolight Did this PR pass all checks? It only shows the appveyor check. Thanks.

@mrcslws Has Victor addressed all your comments? I'm fine with the current status.

@mrcslws
Copy link
Contributor

mrcslws commented Jun 1, 2016

👍

@rhyolight
Copy link
Member

@subutai That's strange... No it didn't. I'm not sure why. When something like this happens, I usually have to push a dummy commit to the remote branch to get the builds to re-trigger. @hernandezurbina You can create a dummy commit like this:

git commit -m "Trigger CI" --allow-empty
git push origin RES217

@hernandezurbina
Copy link
Author

@rhyolight done

@rhyolight
Copy link
Member

Still strange... looks like the CI servers are running, but the local validations are not. This smacks of webhook latency from GitHub.

@rhyolight
Copy link
Member

There we go. I forced a validation manually from the tooling server.

@rhyolight rhyolight merged commit f4d1fe2 into numenta:master Jun 2, 2016
@hernandezurbina
Copy link
Author

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants