-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
NUP-2394: network API code example #3520
NUP-2394: network API code example #3520
Conversation
Only input so far, is if we're going to go all in on YAML model params for this example, we should also change the algorithm quick start to use the same YAML file to load params. |
OK, I'll update the algorithm example. |
…94-network-api # Conflicts: # docs/examples/opf/model_params.py
…xamples (OPF, network API and algo)
6a93c41
to
a771b84
Compare
* Use new network links created in NUP-2396 * Remove customCompute() usage
@rhyolight This PR is blocked by #3526. Changelog:
Once #3526 is merged, this is ready to be reviewed. |
@marionleborgne I also noticed you are writing results to an output file instead of to the console, so the RST will need to be updated to reflect these changes in the descriptive text. |
@rhyolight Actually, I am still writing to the console. I just added writing to an output file, so that I can compare the results of the 3 examples. I'll update the descriptive text. |
Sounds good. I think the examples should be as simple as possible. Writing
to the console is fine.
…---------
Matt Taylor
OS Community Manager
Numenta
On Mon, Apr 10, 2017 at 10:32 AM, Marion Le Borgne ***@***.*** > wrote:
@rhyolight <https://github.com/rhyolight> Actually, I am still writing to
the console. I just added writing to an output file, so that I can compare
the results of the 3 examples. I'll update the descriptive text.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3520 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAA8zhIuzbihVJoZM7mHiXacYNGHcaGnks5rumebgaJpZM4Mznwm>
.
|
After discussion with @rhyolight, the plan is to take out writing to file once we fix the discrepancy between prediction results. Out of the scope of this PR. Task tracked here: https://jira.numenta.com/browse/NUP-2400 |
@@ -0,0 +1,317 @@ | |||
# Type of model that the rest of these parameters apply to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that we want to have the full documentation for all parameters in every model configuration file. Perhaps we could centralize this information somewhere and link to that reference page at the top of this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean other places in the codebase where we have model params with detailed comments? If yes, I agree.
I think this file is a great place for it, because it will show up here (currently python, but soon yaml):
http://docs.numenta.org/quick-start/example-model-params.html
Perhaps we should remove the comments from other places and refer them to the API docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yeah might be fine here. I think the real source of truth should be the algorithm doc strings but those are spread out throughout the code so this can be the place where we include them all. We may want to have references back to the individual algorithms in case they get out of sync.
…94-network-api # Conflicts: # docs/examples/opf/model_params.py
@rhyolight @scottpurdy: can someone review this PR? nothing is blocking it since #3526 was merged last week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that was holding up my feedback is a research discussion on the network api. If we are going to keep saying that that is the way to use things then I don't think we should include a conflicting example (full example without network api) in the docs. But let's hold off on that discussion until after I have that discussion with Subutai, Marcus, others that are interested. And it doesn't have to hold up this PR.
@@ -1,144 +1,171 @@ | |||
import csv | |||
import datetime | |||
|
|||
import yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goes after numpy
from nupic.research.spatial_pooler import SpatialPooler | ||
from nupic.research.temporal_memory import TemporalMemory | ||
from nupic.algorithms.sdr_classifier_factory import SDRClassifierFactory | ||
|
||
_INPUT_FILE_PATH = "../data/gymdata.csv" | ||
_NUM_RECORDS = 3000 | ||
_INPUT_FILE_PATH = '../data/gymdata.csv' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use double quotes here and elsewhere
|
||
sp = SpatialPooler( | ||
# How large the input encoding will be. | ||
inputDimensions=(encodingWidth), | ||
# How many mini-columns will be in the Spatial Pooler. | ||
columnDimensions=(2048), | ||
columnDimensions=(spParams['columnCount']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't want to pull out all the params one at a time. Can you extend spParams with any changes needed and then construct the SP with SpatialPooler(**spParams)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is @rhyolight 's example. I think he pulled the params one by one so that he can describe them in the the quick-start guide: http://nupic.docs.numenta.org/quick-start/algorithms.html
@@ -1,144 +1,171 @@ | |||
import csv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these examples embedded in a web page? Is that why there is no copyright header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
if netResults.error.mean() != algResults.error.mean(): | ||
warnings.warn('Algorithms and Network API predictions are different.') | ||
|
||
plt.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing newline at eof
@@ -0,0 +1,20 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for? It would be nice to have this be a test that just validates there are no exceptions in the examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
- I'll add a unit test to make sure the 3 examples can run without throwing an exception.
- I'll add the other test you are talking about as part of: https://jira.numenta.com/browse/NUP-2401 (Fix results discrepancy between OPF, network API and algorithm API)
- I'll take out the scripts under
docs/util
.
@@ -0,0 +1,84 @@ | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A script to inspect the prediction results of the 3 examples:
- docs/algo/complete-example.py
- docs/opf/complete-example.py
- docs/networkapi/complete-example.py
Adding this info at the top of the file.
* Add unit test to make sure all 3 examples don't thow any exception * Change single quotes to double quotes everywhere * Remove utlity script to plot saved prediction results from all 3 examples * Remove part where examples save predictions to file * Rename networkapi to network for better readability
@scottpurdy thanks for the review, I addressed the feedback. Some things to note:
|
* Make the example code easier to follow in the quick-start section.
import numpy | ||
import os | ||
import yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why yaml? i.e. what benefit does it have over a native python module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all 3 examples we are using a single YAML file with all the params. This YAML is heavily commented to explain each param. Using YAML vs native python module makes it a bit more obvious in the docs that it's a configuration file. Another (small) benefit is that it displays in a more compact manner in the docs and it's easier to read than the native python module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python model params adds the extra requirement of a local import, an extra directory, and an empty __init__.py
file.
@marionleborgne Will review today |
# Random number generator seed. | ||
seed=1956, | ||
seed=spParams["seed"], | ||
# TODO: is this useful? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. Feel free to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marionleborgne If you choose not to make the change above, let me know so I can merge as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and merged. Will revisit.
Fixes #3522.
The code example for the network API is done but the PR is blocked on
NUP-2396
. Currently the example has to callcustomCompute()
on theclassifierRegion
since classification of continuous values is not supported by the network API. I'm moving on to the blocking issue (NUP-2396
) to fix this.Todos, for when I get back to this issue:
customCompute()
and make use of new region links created inNUP-2396
.nupic/docs/examples/params/model.yaml
by adding new params documentation based on feedback collected in google doc.cc @rhyolight