add OpenNARS-style Distributor in Bag #84
Conversation
|
Hi Maxim, I will need more info about what is the Distributor |
|
@ccrock4t you can see the code from OpenNARS. As the comments suggest (I will add them to this PR shortly)
And
This is all following our discussion over email, regarding selection probability in bag being propositional to priority. Without this change, the result of
After the change it looks like this which I believe is more along the lines of what we want
|
|
In terms of speed, it seems like that is the cost of using this algorithm. If we can find out the name of the algorithm, maybe there are discussions online about improving it? |
|
|
||
| if self._is_current_level_empty(): | ||
| self._move_to_next_nonempty_level() | ||
| if self._is_current_level_empty() or self.currenCounter == 0: |
There was a problem hiding this comment.
Can you help me understand why we need currentCounter? It seems to hold the most recent index of an Item taken from a Bucket. What is special about index 0 to make Bag change its current Level?
There was a problem hiding this comment.
I suppose we need to change the level at some point, but why not at every Take()?
There was a problem hiding this comment.
Can you help me understand why we need currentCounter? It seems to hold the most recent index of an Item taken from a Bucket. What is special about index 0 to make Bag change its current Level?
So this is a fun one :) I basically just converted Java to Python and adapted it, so there may be better ways to write all this stuff.
This is related to the THRESHOLD parameter I was asking about over email. It seems similar to the PyNARS take_in_order as far as I understand. In OpenNARS if currentLevel is below THRESHOLD, then currentCounter will be set to 0 and at next call to take we will go inside the conditional, otherwise we will continue taking from the current level until empty.
I suppose we need to change the level at some point, but why not at every Take()?
The way it's currently implemented it is indeed going to change levels at every take since take_in_order is set to True so the implementation is not complete in that regard. In OpenNARS I also wasn't able to find any instances where it would fire off the level completely as THRESHOLD is always set to 1. So I'm not entirely sure what the use case is for this property or how to fully reconcile the two implementations since Bag in PyNARS has quite a few other methods and the code is different compared to OpenNARS. If you guys want, we can have a design discussion and fully document the Bag implementation and figure out all of these edge cases. I would need your help though.
As it is yeah, if take_in_order is set to False then the logic is kinda weird. There isn't a direct match for it in the Java code so looking to you guys for suggestions on how to handle this.
There was a problem hiding this comment.
I see, so the graph is showing the behavior when the level changes every take(), which is good because it matches our expectation. I am not sure about take_in_order itself, but in my opinion for take() we should choose a random level every time, just remove the self.currenCounter unless @bowen-xu sees it should be used for take_in_order.
I agree it is a good idea to document Bag more thoroughly. We could put more info in the Conceptual Design document we have going
There was a problem hiding this comment.
take_in_order does not appear in the conceptual design. It can be removed from the code.
I didn't really read the code of Bag in OpenNARS 3.0.4; but I only referred to NARS-Python before. I just found that the previous implementation of Bag in PyNARS is not consistant with OpenNARS.
I suggest that we shall faithfully adopt the orignial design of OpenNARS in terms of Bag. So thanks @maxeeem for helping us consider this issue carefully!
@maxeeem Why not just re-implement the Bag data-structure in python, so that it could be fully the same as the java version?
There was a problem hiding this comment.
I can look into aligning the implementations and porting it from Java to Python. I did notice that Buffer uses take_in_order=False so we'll need to examine that as well and I imagine Channels which inherits from Buffer.
|
It seems a Distributor with value My initial thoughts: the EDIT: it will probably not be that simple |
| self.levels = tuple(list() for i in range(self.n_levels)) # initialize buckets between 0 and capacity | ||
|
|
||
| self.currentCounter = 0 |
There was a problem hiding this comment.
the "lower camel case" style is better
There was a problem hiding this comment.
Thanks for pointing this out. Old habits are hard to break :) I'll push up a fix shortly.
@ccrock4t Is the slowdown significant? I found that the initialization takes only around 8ms in my laptop. |
Not for the current default number of buckets (100), but the time for initialization could become an issue if we need to significantly increase the granularity of Bag (e.g., to 10,000 or more). Since it is only a 1-time operation, the slowdown would not really impact user experience during NARS operation. Only on startup. However the memory also takes up |
|
@ccrock4t |
I agree it is OK for now, since we only tend to use 100 buckets. But, if we want to try more a lot more buckets in the future, we should revisit this. |
See #83
I added the distributor but looking for your suggestions to see if we can improve the implementation. The issue is that initializing the Distributor object is not exactly quick, there are multiple loops in the
__init__method. To avoid recalculation I added a cache in the form of a static methodnewbut as I am myself new to python, perhaps you guys can think of better ways to optimize the calculations. With this change tests run slower but still less than 1 min.