-
Notifications
You must be signed in to change notification settings - Fork 75
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
Example implemented using NetworkAPI #760
Conversation
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.
Thanks David, it's great to have this example covered in NetworkAPI 👍
- I think I got the reason for "slowness"
- bonus: would be great if this example can read from a CSV, and use real hotgym data file (see hotgym.py)
std::string encoder_parameters = "{size: " + std::to_string(DIM_INPUT) + ", activeBits: 4, radius: 0.5, seed: 2019 }"; | ||
std::shared_ptr<Region> region1 = net.addRegion("region1", "RDSERegion", encoder_parameters); | ||
std::shared_ptr<Region> region2a = net.addRegion("region2a", "SPRegion", "{columnCount: " + std::to_string(COLS) + "}"); | ||
std::shared_ptr<Region> region2b = net.addRegion("region2b", "SPRegion", "{columnCount: " + std::to_string(COLS) + ", globalInhibition: true}"); |
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.
regarding parameters, these setings use SPs' etc default params, I think it'd be nice to use:
- I think the best would be if you also add SensorRegion to show reading a CSV file, so to replicate
hotgym.py
(uses the real hotgym data, also then use its params)
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.
Ok, I did not even think to pattern it after hotgym.py. I assumed it was the same as HelloSPTP.cpp. I will check that out.
It looks like I need to make some more Regions.
And I like the idea of setting up the parameters in a table. What if I used a Yaml table? There is also a problem in knowing where the .csv file exists. The program cannot assume it is in the same folder as the executable...most likely not. Have to think about that. |
probably yes, also SDRClassifier.
this should be already available in
would be nice to have. Date enc might see some larger changes, but who knows when.. (there's an issue for that)
do you mean a YAML file with the params? My ideal goal would be that Algorithms, and Regions support additional constructors with a (yaml/json) config file, so you can change settings w/o recompiling.
What about a py Region for plotting if python is avail, and we can sort out plotting with no py later?
this should be a minor issue. Expect the file path given as arg. And default the data can be copied by cmake to the bin/ folder (?) |
Yes, anomaly is there but how do I get AnomalyLikelihood from it? I will probably need to add some code someplace to get that.
yes. I could pass a Yaml File into the addRegion call, or by expanding this a little, pass one yaml file to Network and it could define the regions to add as well as the parameters to apply to them.
That was my thinking. Although when the program runs it knows only the default folder and that can be any place. It does not know where the repository is or even if one exists. In linux, you use cmake to do an install step before running the programs so more than likely, if you built from sources, the executable will be in the build/Release/bin folder. At least on Windows, if you do a debug build within MSVC it does not do the install step first so it executes it from someplace down in build/scripts/?? so in this case it will not know where to find the csv file. anyway, I will think of something. |
So, it looks like I need to set this PR aside until I can do the following:
|
Oh, the SDRClassifier is actually the Predictor. Very confusing. |
TM has a param
how about wrapping this up to make it HelloSPTP alternative, as originaly intended, merge, and then extend to the hotgym.py functionality? steps 1,4 are imho incorrect, 5,6 optional (would suit a separate PR each).
it's in the same file, if you split it into standalone, it'd be fine. |
It appears that the HelloSPTP.cpp example has nothing to do with hotgym so the hotgym.cpp is a bad name. Perhaps it should be called sine_wave or something. So you think I should also create a HelloSPTP version using NetworkAPI in addition to the real hotgym. I guess I could. |
well, it does. It uses the same processing pipeline: SP-TM-Anomaly/prediction. ATM it's just not yet as complete as the py version.
no, I was suggesting to merge this as-is (=similar to HelloSPTP) for now, and then build other NetworkAPI additions on it, to get closer to the hotgym.py functionality |
They are so different, I think it better to have two separate examples. |
The anomaly always comes out as 1. I still have the SP (local) in the program. Since that is broken, let me try taking that out and see how much faster it runs. |
yes, it runs much better without SP (local). SP (global) takes about 10 seconds to initialize (Windows, debug mode) but I guess that is ok. |
SP with local inhibition is not broken, it's just slow. The usefulness us now questionable, but in a way the produced SDRs are of a better quality. We should try speeding it up. |
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.
Appart from the last question about why the resutls are bad/"bad", this looks fine to me!
Thank you David
PS: I think the follow up PRs could build/modify these files and turn them into the "real" hotgym.py alike.
Thanks for approving this, but I think this should remain as a Hello World type example and I should build a second example that matches the hotgym.py example. Both would have value. I was not trying to match the results of HelloSPTP in this PR but perhaps I should. I am computing the sine differently and I may have different parameters. So let me see if I can make them more the same. The fact that both are bad is also a problem. This is probably a user's first exposure to htm.core, at least that is its purpose and it should have at least reasonable results. We control the example and we should pick an example that shows what htm can do. To not do so is embarrassing. That is bugger than what should be done in this PR so lets put that in another PR but it should be done. |
if not 1:1 deterministic results, similar quality of results would be good!
ok, as a separate issue, we should look at the results & params, it's true that avg 0.4 anomaly on a sine wave in 5000 steps isn't stellar. (I don't know if the random noice isn't too strong there etc) This PR, feel free to merge as is 👍 |
At the moment I am not even close. |
Found some things that needed correcting:
|
Additional things we might want to consider:
|
@breznak I will need another review when you find time. I will be traveling starting tomorrow, returning probably Sat. Happy Thanksgiving everyone. |
|
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.
Great to have the matching results now, thanks Dave 👍
Happy thanksgiving everyone! 🦃
Please see small changes below,
@@ -47,18 +48,23 @@ namespace htm { | |||
resolution: {type: Real32, default: "0.0"}, | |||
category: {type: Bool, default: "false"}, | |||
seed: {type: UInt32, default: "0"}, | |||
sensedValue: {type: Real64, default: "0.0", access: ReadWrite }}, | |||
noise: {description: "amount of noise to add to the output SDR. 0.01 is 1%", |
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.
nice idea to have the noice included in the encoder 👍
Maybe for later, it'd be useful to have a "NoisyRegion", so that it can be applied to/after any layer. That is similar to dropout PR #535
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.
Good idea.
@@ -336,7 +343,7 @@ Spec *TMRegion::createSpec() { | |||
NTA_BasicType_UInt32, // type | |||
1, // elementCount | |||
"", // constraints | |||
"8", // defaultValue | |||
"10", // defaultValue |
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.
so the defaults now match the Algo version (?) It's good that thanks to this example, we've validated that people using NetworkAPI get the same quality out of the box 👍
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, I changed all NetworkAPI defaults to match the Algorithm defaults. This required a manual eye-ball compare. We don't have a way to write a test that can check if the defaults are the same in all cases.
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 a new example program.
This seems very slow. Can someone check my parameters?