forked from numenta/nupic.core-legacy
-
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
Cleanup exceptions #742
Merged
Merged
Cleanup exceptions #742
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a188e8b
Log: simplify NTA_ASSERT macro
breznak 6605c05
Log: remove NTA_LDEBUG macro, unused
breznak 0137215
Log: deprecate LogItem
breznak ae66bf1
Exception: implement operator<< and remove LoggingException
breznak 1b46714
Log: remove LogItem as unused
breznak c1f6ed6
Network.setLogLevel fix in bindings
breznak 34f9b6f
Network: make setLogLevel static
breznak 4560ce7
TMRegion: remove the hack for NTA_DEBUG
breznak 2149e83
WIP trying to fix linked regions in TMRegion
breznak fac5da7
Merge branch 'master_community' into cleanup_exceptions
breznak 2552213
TMRegion: compute all outputs
breznak 17fdb7c
Made the LogLevel variable thread local
dkeeney File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@dkeeney this is something I'd like to consult with you. Actually the problem I'm hitting here seems to be a bug in TMRegion,
a) I'm removing the "is Debug" part of the check, as it was supposed to be a helper for debugging, but breaks the logic (tests expecting such behavior are wrong)
b) there's a problem with requiring the
hasOutgoingLinks()
, as:This is the test failing:
The network is
Encoder->SP->TM
,I was surprised only the TM check is failing, but it is because the TM is a "leaf" (it's last part of the Network), and we have the condition
if (out && out->hasOutgoingLinks() )
.now, we want a similar check (to "has outputs") so that we only compute for connected regions. Using
getInput("name").hasIncomingLinks()
would work for TM in this example, but would fail for the root (sensor/encoder).So do we want a combination and determine if XXX (what is the "predictedActiveCells" called, it's not a region per se...?) is connected by
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 am having a problem understanding your post on my cell phone.
I will have to check it out later when I return.
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 logic is correct. Although we need someplace else to put isDebug() since you removed LogItem. This is part of the trace facility. When the debug mode is activated by an application by calling setLogLevel(LogLevel_Verbose), the trace should show each region that is executed and the outputs.
For the trace facility to work, we need at least the following:
This trace facility is extremely useful when debugging a problem with region execution and or linking. In particular, which data is being passed at which times and the order that the regions are executed. This is another case where we need more documentation and explanation of the 'features' that are available.
Oh, and the 'Verbose' facility that I used with some of the Unit Tests does not use the trace facility.
TMRegion has several 'optional' outputs. For the 'optional' outputs, if there is no outgoing connection, then the output is not generated. So, to get the trace in Verbose mode we should generate the output data even though it is not being sent to an output. That is what the above 'if' statement does.
However, recently we exposed the ability to access the output buffers directly with region.getOutput(name).getData(). This causes a problem because if there is no outgoing link the optional outputs cannot be accessed in this way. Perhaps to be on the safe side we should always produce all outputs even if they are not used. In that case we can remove isDebug( ) and remove the entire 'if' statement at the top but the cost is lower performance when the outputs are not needed.
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 have no idea what you are trying to say here. Could you re-phrase it?
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 what I wanted to disprove. Debug is useful but we should not change regions' output based on it (as the output is what tests check)
we have these, as:
NTA_LOG_LEVEL
Network.setLogLevel()
(but they do have access to the global variable)NTA_LOG_LEVEL == htm::LogLevel::LogLevel_Verbose
can we use NTA_DEBUG?
going back to what I was trying to prove wrong.
and
I'm not sure what the "optional" output is in this context, but this is the bug. In the test mentioned, the Network looks like
encoder -> SP -> TM
with TM'sgetOutput("predictedActiveCells")
apparently being optional. But users (and our tests) can expect the output and query for it, and it's empty.So we should either:
E A->B->C
: do not compute E). Now we compute all ofB
because it has out link to C. But we do not compute optional outputs ofC
(as it has no outgoing links)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.
The above variable is not Global. It will create a different instance in every .cpp in which log.hpp is included. So it will work only if the logLevel is set and used in the same .cpp.
To make this global you need to declare it
extern
in the .hpp and someplace in some .cpp it must be declared without thestatic
. OR instantiate a class that has this as a class variable...a more C++ way of doing things.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.
Oh, and you called the outputs 'regions'. That confused me at first. A region is the wrapper around an algorithm. A region can have multiple outputs.
Keep in mind that when we start doing multi-threading, this global variable will not be very useful. A better way to handle trace when doing threads is to have the global in the thread-global space so that each thread's trace can be controlled independently. That is why I wanted to use a function or Macro to set the logging variable rather than assigning a value to it.
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.
you're right, let's not be smarter than the user, the implementation will also be simpler.
I agree. Are we able to do that w/o a significant change to how the NTA_WARN etc macros are used? Or we make a big step and deprecate the macros and use the Log class you've charted (like in java Logging works) ?
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.
Oh, I don't propose deprecating the macros, just change the macros to use the Log class.
If you declare the class as a static variable in the Log.hpp file it will create a separate instance of it in each .cpp in which Log.hpp is included. But that is ok since we only need state from the class variable which will be global in scope. An alternative is to go head and use the thread global space to store the LogLevel state (prefered). Either way we would not need a Log.cpp. Everything could be hidden by Macros so that they can be changed without changing the API.
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.
Oh, there is a new feature in C++11 that I did not know about. Its called
thread_local
. So we don't have to get the thread id and allocate the thread specific space for it. Its done for us.See https://en.cppreference.com/w/cpp/language/storage_duration
With this you don't need the Log class.