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.

Sign up for GitHub

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

Jump to bottom

CLAModel name changed to HTMPredictionModel #3516

Merged
merged 11 commits into from
Apr 14, 2017
Merged

CLAModel name changed to HTMPredictionModel #3516

merged 11 commits into from
Apr 14, 2017

Conversation

rhyolight
Copy link
Member

fixes #3515

@rhyolight rhyolight requested a review from scottpurdy April 3, 2017 23:52
@scottpurdy
Copy link
Contributor

As discussed offline, CLAModel is an OPF model (subclass of nupic.frameworks.opf.model.Model) that runs HTM. So HTMModel would be a better name.

@rhyolight
Copy link
Member Author

I kindof like SPTMModel. How does this feel @mrcslws @scottpurdy ?

@rhyolight rhyolight changed the title CLAModel name changed to OPFModel CLAModel name changed to SPTMModel Apr 4, 2017
@rhyolight rhyolight requested a review from mrcslws April 4, 2017 21:30
mrcslws
mrcslws approved these changes Apr 4, 2017
View reviewed changes
Copy link
Contributor

@mrcslws mrcslws left a comment

Choose a reason for hiding this comment

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

Spittem!

@rhyolight rhyolight requested review from subutai and removed request for scottpurdy April 4, 2017 23:04
@rhyolight
Copy link
Member Author

Will wait for @subutai before merging.

subutai
subutai requested changes Apr 11, 2017
View reviewed changes
Copy link
Member

@subutai subutai left a comment

Choose a reason for hiding this comment

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

Looks good overall. This is a major incompatible change - recommend warning the community.

examples/opf/experiments/anomaly/spatial/10field_many_balanced/UNDER_DEVELOPMENT
@@ -34,7 +34,7 @@ from nupic.frameworks.opf.expdescriptionhelpers import (
applyValueGettersToContainer,
Copy link
Member

Choose a reason for hiding this comment

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

If this is marked as UNDER_DEVELOPMENT can we just delete it?

Copy link
Member Author

@rhyolight rhyolight Apr 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

There are a bunch of "UNDER_DEVELOPMENT" examples. I've removed them in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - might want to merge in master - will likely be merge conflicts.

docs/source/quick-start/opf.rst Outdated
@@ -34,7 +34,7 @@ using the :class:`.ModelFactory`.

.. literalinclude:: ../../examples/opf/create-model-example.py

The resulting ``model`` will be an instance of :class:`.CLAModel`.
The resulting ``model`` will be an instance of :class:`.SPTMModel`.
Copy link
Member

Choose a reason for hiding this comment

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

SPTM is not too informative. Should we consider calling it StreamingModel or PredictionModel?

Copy link
Member Author

Choose a reason for hiding this comment

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

@subutai @mrcslws @scottpurdy I would like it if you guys could talk in the office and come up with a name you agree on. Once you have one, I will update the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@subutai - "Prediction" and "streaming" aren't good because all OPF models are streaming and prediction. One really important thing to capture is that this is HTM-related since other models may be for reference (like TwoGramModel). While "HTMModel" might be ok, we may split out certain variants into their own classes rather than shoe-horning all HTM-variants into a single OPF model.

That is how we got to "SPTMModel" but definitely open to other ideas.

Copy link
Member Author

Choose a reason for hiding this comment

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

And spittem is fun to say

src/nupic/frameworks/opf/sptmmodelcallbacks.py Outdated
@@ -35,12 +35,12 @@ def claModelControlEnableSPLearningCb(claModel):

See also claModelControlDisableSPLearningCb.
Copy link
Member

Choose a reason for hiding this comment

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

The text prefix claModel was not replaced. Shows up in many places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, waiting for a final name before I commit anything.

@rhyolight
Copy link
Member Author

@subutai Warned the community here: https://discourse.numenta.org/t/warning-0-7-0-breaking-changes/2200/4

This PR is ready to merge if we are ok with SPTMModel.

@rhyolight rhyolight changed the title CLAModel name changed to SPTMModel CLAModel name changed to HTMPredictionModel Apr 12, 2017
@rhyolight
Copy link
Member Author

Decision: HTMPredictionModel. Changes coming.

@rhyolight
Copy link
Member Author

I just realized this also means we need to update the CLA model type in model params:

    # Type of model that the rest of these parameters apply to.
    'model': "CLA",

More changes coming to lots of example files, and the ModelFactory to identify it with HTMPrediction.

subutai
subutai approved these changes Apr 13, 2017
View reviewed changes
src/nupic/regions/CLAClassifierRegion.py
# get to cleaning up the clamodel.py file.
# being called at the same time as the compute() method. Only compute()
# should be called via network.run(). This flag will be removed once we
# get to cleaning up the htmpredictionmodel.py file.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the file be htm_prediction_model.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just following previous convention. Should I change both the module name and file name?

Copy link
Member

Choose a reason for hiding this comment

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

I thought having the _ was our filename convention (not module names), but you are right that the OPF directory doesn't seem to follow that. Perhaps best handled in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok: #3546

examples/opf/experiments/anomaly/spatial/10field_many_balanced/UNDER_DEVELOPMENT
@@ -34,7 +34,7 @@ from nupic.frameworks.opf.expdescriptionhelpers import (
applyValueGettersToContainer,
Copy link
Member

Choose a reason for hiding this comment

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

Thanks - might want to merge in master - will likely be merge conflicts.

rhyolight
rhyolight commented Apr 13, 2017
View reviewed changes
..._test/experiments/backwards_compatibility/a/savedmodels_2012-10-05/DefaultTask.nta/model.pkl
sb.
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I have left this pickle alone?

@rhyolight
Copy link
Member Author

@scottpurdy @subutai Ready for final review. This might break regression tests. If so I will fix tomorrow.

@rhyolight rhyolight merged commit 914ada4 into numenta:master Apr 14, 2017
@rhyolight rhyolight deleted the clamodel branch April 14, 2017 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottpurdy scottpurdy scottpurdy left review comments

@subutai subutai subutai approved these changes

@mrcslws mrcslws mrcslws approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Name change: CLAModel
4 participants
@rhyolight @scottpurdy @mrcslws @subutai