New YAML parser interface #715

dkeeney · 2019-10-12T20:27:18Z

Continuation of PR #282

src/htm/ntypes/Value.hpp

breznak · 2019-10-13T20:52:38Z

src/htm/regions/SPRegion.cpp

@@ -46,7 +45,7 @@ SPRegion::SPRegion(const ValueMap &values, Region *region)
  args_.columnCount = values.getScalarT<UInt32>("columnCount", 0);
  args_.potentialRadius = values.getScalarT<UInt32>("potentialRadius", 0);
  args_.potentialPct = values.getScalarT<Real32>("potentialPct", 0.5);
-  args_.globalInhibition = values.getScalarT<bool>("globalInhibition", true);
+  args_.globalInhibition = values.getScalarT<bool>("globalInhibition", false);


please use True as default, local inh is quite slow, and global is the default in the SP class.

please use True as default,

Well...the problem is that this PR does change behavior slightly. In the past the defaults from the Spec were incorporated in values. The default given here was never used. The new behavior is that the defaults in the Spec are NOT included in values and therefore this default is now being used.

What I did was change these values to be the same as what was given in the Spec so that the default parameters passed on to the SP algorithm are unchanged.

Now, if you want to change the default to True in this case, then you must also change the expected results in the tests. I agree that the default here should be the same as the defaults used by SP but that sounds like a separate PR.

What I did was change these values to be the same as what was given in the Spec so that the default parameters passed on to the SP algorithm are unchanged.

ok, thanks for explanation, this is fine. also for other on the same topic 👍

I agree that the default here should be the same as the defaults used by SP but that sounds like a separate PR.

yep, let's keep that for another pr

src/htm/regions/SPRegion.cpp

src/htm/regions/ScalarSensor.cpp

src/htm/regions/TMRegion.cpp

src/test/unit/engine/CppRegionTest.cpp

dkeeney · 2019-10-15T21:49:00Z

@breznak I need your help. I know I must be doing something stupid but I cannot find it.

When I run the python unit tests I get 4 errors in NetworkTest.py. lines 164, 244, 264, 328. All three are the same error.

>     r = net.addRegion("test", "py.LinkRegion", "")
E     RuntimeError: TypeError: __init__() got multiple values for argument 'self'

J:\Projects\AI\htmG\bindings\py\tests\regions\network_test.py:328: RuntimeError

I need someone with more Python experience to take a look at this. It could be something to do with the bindings for addRegion( ) but I made no changes to it.

Where I did make changes is in bindings\py\cpp_src\plugin\PyBindRegion.cpp where I changed the building of the parameters when C++ code calls Python implemented regions. The original logic did not allow nesting of parameter structures. Calls to addRegion( ) would cause this to be called if the region is written in python.

I will appreciate any help I can get on this.

dkeeney · 2019-10-16T21:28:23Z

I gave up too soon...found the problem.
Of course it was someplace I was not expecting it to be...in the code that processed the Spec being read from the python code. The changes in my code exposed a bug over in that code.
Anyway... I working on making the corrections....

breznak · 2019-10-17T08:19:18Z

Do you intend to keep options for both libraries, or we decide for one over another?

dkeeney · 2019-10-17T12:50:58Z

Do you intend to keep options for both libraries, or we decide for one over another?

I don't know yet.

yaml-cpp still has the crash problem.
libyaml does not have the crash problem.
yaml-cpp is based on YAML 1.2
libyaml is based on YAML 1.1

At no time will we have both libraries compiled at the same time.
But I am still doing A - B tests to see which one seems to work best for us.
Eventually I will probably decide on one and remove the interface for the other but for now I would like to keep the options open.

dkeeney · 2019-10-17T13:14:56Z

There are still some things I do not like about the Value class.

its name, 'Value'. I am not imaginative enough to come up with a better name.
I use this 'assigned_' pointer which I do not like. Perhaps there is a way to do this with shared_ptr.

At the moment I am working on PyBindRegion.cpp and trying to get it to work with the new Value class. The GridCellLocationRegion.py uses parameters orientation and scale which are arrays of floats. That was not handled correctly.

But in general, the Spec limits the complexity of the parameter structure to flat mapped values or one dimensional arrays. I would expect new Regions (or encoders) to need a much richer parameter structure. But for now we are limited to this.

dkeeney · 2019-10-19T00:01:46Z

I want to tighten up the Value class before we merge.
I want to see if I can get rid of the assigned_ pointer. It might simplify a lot of code if the zombie object can be moved into the std::map without changing its address...so that things that point to it do not need to change their pointer after an assignment.

breznak

Big congratulations for making this work, David! 🎉

I'm happy to see the cleanup and the new code has a really helpful docs 👍

small comments below
README might need updates as it mentions libyaml-cpp
I'm looking forward to the simplification with pointers you're mentioning
are we completely happy with libayml, or it too has its pros & cons?

Thank you!

breznak · 2019-10-19T08:15:58Z

external/libYaml.cmake

+elif(EXISTS ${REPOSITORY_DIR}/build/ThirdParty/share/libyaml.tar.gz)
+    set(URL ${REPOSITORY_DIR}/build/ThirdParty/share/libyaml.tar.gz)
+else()
+    set(URL "https://github.com/yaml/libyaml/archive/master.zip")


please use a fixed release/tag/commit

please use a fixed release/tag/commit

The current release is 0..2.2 but it did not build.
so as mentioned in the comments, we have to use the master repository until they release again.

we have to use the master repository until they release again.

yes, but use specific commit that works, so the "version" is fixed

How do I do that? I think I can only have access to whatever is the latest in the archives.

src/htm/ntypes/Value.cpp

breznak · 2019-10-19T08:24:35Z

src/htm/ntypes/Value.hpp

+|  Value object - a container for data structures that have been parsed
+|  from a YAML or JSON string.  It can hold any structure that can be
+|  parsed from these but does not implement Alias or reference features.
+|  YAML is a superset of JSON format but we only use the JSON compatible features.


could we use a JSON parser then? (not that we need, if libyaml works good)

could we use a JSON parser

Yes, we could. But I would rather handle both JSON and Yaml.

dkeeney · 2019-10-20T13:40:03Z

I was able to get rid of the assigned_ feature so this is a little cleaner. Not sure it is much simpler however.

The zombies make it difficult to follow the code but I could not think of a better way to have a place to hold the keys and parent from a lookup until an assignment is made. yaml-cpp and rapidyaml used zombies as well. libyaml did not have a tree.

While I was at it, I added some thread locks so I think this should be threadsafe. I did not confirm that it is threadsafe...that would be a major test effort that is probably not worth it at this point.

dkeeney · 2019-10-20T15:20:31Z

The downloader for yaml-cpp is still in the package although not used. I would like to keep that for a little while to make sure that libyaml continues to work ok for us.

With the addition of this package, the end users of the library should not see anything different. However it does a few things for us.

cleans up some rather ugly code.
encapsulates the yaml parser so that in the future we have the option of switching to a different yaml parser with minimum effort if we find there is a problem.
Yaml (and JSON) parsing no longer uses the Spec so it can be used for things other than Region parameters. The Value structure is a universal container for parsed Yaml.
Moves the processing of parameters with the region Spec closer to the Region implementation so that the Region can have more flexibility in the layout of the parameters. It can now do nested sequences and maps.

In general it was much more work that should have been devoted to a low priority bug. :-)

dkeeney · 2019-10-20T16:31:12Z

Oh, another idea as to how to simplify some more...
...the parent_ field points to the parent's core not the parent Value object.
Let me see if that helps.

dkeeney · 2019-10-20T18:23:18Z

Yes, using the parent's core as the parent_ pointer on each node helped somewhat.

breznak

This does look pretty good!

Thanks for the improved doc & the thread-safety is a nice touch too!

New YAML parser interface

9c81a7c

dkeeney mentioned this pull request Oct 12, 2019

Yaml Crash Problem WIP #282

Closed

dkeeney self-assigned this Oct 12, 2019

dkeeney added low NetworkAPI in_progress labels Oct 12, 2019

dkeeney added 2 commits October 13, 2019 06:18

now compiles on linux, but still crashes

a1c831a

working in windows. Back to Linux to try

7c1f43a

breznak reviewed Oct 13, 2019

View reviewed changes

dkeeney added 6 commits October 15, 2019 11:13

switched to libyaml

60e4f49

working on Linux (except for python test)

8177ac3

remove unused variable

8cffa67

removed another unused variable

5699be1

removed another unused variable

c8d38ce

made some move()'s

1ad03f9

dkeeney added 2 commits October 15, 2019 17:22

added documentation

aa85784

corrected spelling in documentation

2f6088f

dkeeney added 2 commits October 18, 2019 14:40

compiles clean with libyaml on Windows.

12e288b

fixed some warnings

77bfdc4

breznak previously approved these changes Oct 19, 2019

View reviewed changes

removed assigned_, added threadsafe.

c764a46

dkeeney dismissed breznak’s stale review via c764a46 October 20, 2019 13:30

some simplifications.

6e902a6

breznak approved these changes Oct 21, 2019

View reviewed changes

dkeeney merged commit 23168cf into master Oct 21, 2019

dkeeney deleted the yamlProblem2 branch October 21, 2019 12:55

breznak mentioned this pull request Oct 21, 2019

Yaml-cpp bug in debug mode #218

Closed

1 task

dkeeney mentioned this pull request Oct 27, 2019

Parsing region parameters without requiring the Spec. #664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New YAML parser interface #715

New YAML parser interface #715

dkeeney commented Oct 12, 2019

breznak Oct 13, 2019

dkeeney Oct 14, 2019

breznak Oct 15, 2019

dkeeney commented Oct 15, 2019

dkeeney commented Oct 16, 2019

breznak commented Oct 17, 2019

dkeeney commented Oct 17, 2019

dkeeney commented Oct 17, 2019

dkeeney commented Oct 19, 2019

breznak left a comment

breznak Oct 19, 2019

dkeeney Oct 19, 2019

breznak Oct 19, 2019

dkeeney Oct 19, 2019

breznak Oct 19, 2019

dkeeney Oct 19, 2019

dkeeney commented Oct 20, 2019

dkeeney commented Oct 20, 2019

dkeeney commented Oct 20, 2019

dkeeney commented Oct 20, 2019

breznak left a comment

New YAML parser interface #715

New YAML parser interface #715

Conversation

dkeeney commented Oct 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkeeney commented Oct 15, 2019

dkeeney commented Oct 16, 2019

breznak commented Oct 17, 2019

dkeeney commented Oct 17, 2019

dkeeney commented Oct 17, 2019

dkeeney commented Oct 19, 2019

breznak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkeeney commented Oct 20, 2019

dkeeney commented Oct 20, 2019

dkeeney commented Oct 20, 2019

dkeeney commented Oct 20, 2019

breznak left a comment

Choose a reason for hiding this comment