Replace dump() with define str in Encoders. Issue #1518 #3559

abhinavrai44 · 2017-04-21T13:59:47Z

Contributes to #1518
Fixes #1518 (to satisfy "issue checker")

abhinavrai44 · 2017-04-23T04:20:58Z

@breznak Plz Check

breznak

Nice work @abhinavrai44 👍

There are some minor items in the review that need to be fixed.
Also I have 2 questions:

did you replace relevant uses of the dump in code, if any? (we should see that as error when you remove the dump() method)
are there any other encoders that dont implement str()?

Keep up the good work,
Cheers!

breznak · 2017-04-23T11:32:59Z

src/nupic/encoders/random_distributed_scalar.py

@@ -441,18 +442,8 @@ def _permutation(n):


  def dump(self):


you forgot to replace dump with __str__ here too

breznak · 2017-04-23T11:33:45Z

src/nupic/encoders/random_distributed_scalar.py

-    print "  name:       %s" % self.name
-    if self.verbosity > 2:
-      print "  All buckets:     "
-      pprint.pprint(self.bucketMap)


keep the logic for verbosity here.

breznak · 2017-04-23T11:34:10Z

src/nupic/encoders/scalar.py

@@ -700,18 +700,8 @@ def closenessScores(self, expValues, actValues, fractional=True):


  def dump(self):


ditto, replace with __str__

abhinavrai44 · 2017-04-23T18:02:14Z

Sorry for that silly mistake of not replacing dump. The answers to your questions are:-

random_distributed_scalar.py did call dump() in init function which I have removed.
I have only replaced dump with str. But yes there are some classes which do not contain any function to print the object. If needed I can add str to those classes as well.

abhinavrai44 · 2017-04-28T10:50:14Z

@breznak Plz check

breznak

It looks very good!
A few more last touches please and we can merge!
@rhyolight I'll merge soon if you have no objections

breznak · 2017-04-28T11:54:07Z

src/nupic/encoders/random_distributed_scalar.py

    if self.verbosity > 2:
-      print "  All buckets:     "


I'd do something as:

str = "RandomDist...." if verbosity >2: str += "All buckets: ".... return str

breznak · 2017-04-28T11:55:38Z

src/nupic/encoders/random_distributed_scalar.py

@@ -152,9 +154,6 @@ def __init__(self, resolution, w=21, n=400, name=None, offset=None,
    else:
      self.name = "[%s]" % (self.resolution)

-    if self.verbosity > 0:


lets keep the "verbosity" logic. Now with the str you can use print(self) instead of dump()

breznak · 2017-04-28T11:57:20Z

src/nupic/encoders/random_distributed_scalar.py

@@ -23,6 +24,7 @@
 import numbers
 import pprint


pprint seems as not needed anymore

rhyolight · 2017-04-28T14:14:17Z

src/nupic/encoders/adaptivescalar.py

-
+  def __str__(self):
+    return "AdaptiveScalarEncoder\n  min: %f\n  max: %f\n  w: %d\n  n: %d\n  resolution: %f\n  radius: %f\n  periodic: %f\n  nInternal: %f\n  rangeInternal: %f\n  padding: %d" \
+            % (self.minval, self.maxval, self.w, self.n, self.resolution, self.radius, self.periodic, self.nInternal, self.rangeInternal, self.padding)


Please use the max line length of 79 chars specified in PEP-8.

Also use the string.format function for string formatting.

rhyolight · 2017-04-28T14:15:24Z

src/nupic/encoders/coordinate.py

-    print "  w:   %d" % self.w
-    print "  n:   %d" % self.n
+  def __str__(self):
+    return "CoordinateEncoder:\n  w:   %d\n  n:   %d" %(self.w, self.n)


Use string.format.

rhyolight · 2017-04-28T14:15:32Z

src/nupic/encoders/geospatial_coordinate.py

-    print "  w:   %d" % self.w
-    print "  n:   %d" % self.n
+  def __str__(self):
+    return "GeospatialCoordinateEncoder:\n  w:   %d\n  n:   %d" %(self.w, self.n)


Use string.format.

rhyolight · 2017-04-28T14:15:46Z

src/nupic/encoders/random_distributed_scalar.py

@@ -1,3 +1,4 @@
+


Remove unnecessary newline.

rhyolight · 2017-04-28T14:16:17Z

src/nupic/encoders/random_distributed_scalar.py

+          % (self.minIndex, self.maxIndex, self.w, self.getWidth(), self.resolution, str(self._offset), self.numTries, self.name, json.dumps(self.bucketMap))
+    else:
+      return "RandomDistributedScalarEncoder\n  minIndex:   %d\n  maxIndex:   %d\n  w:          %d\n  n:          %d\n  resolution: %g\n  offset:     %s\n  numTries:   %d\n  name:       %s" \
+          % (self.minIndex, self.maxIndex, self.w, self.getWidth(), self.resolution, str(self._offset), self.numTries, self.name)


Same comments, string.format and line length.

rhyolight · 2017-04-28T14:16:23Z

src/nupic/encoders/scalar.py

-
+  def __str__(self):
+    return "ScalarEncoder:\n  min: %f\n  max: %f\n  w:   %d\n  n:   %d\n  resolution: %f\n  radius:     %f\n  periodic: %s\n  nInternal: %d\n  rangeInternal: %f\n  padding: %d" \
+            % (self.minval, self.maxval, self.w, self.n, self.resolution, self.radius, self.periodic, self.nInternal, self.rangeInternal, self.padding)


Same comments, string.format and line length.

abhinavrai44 · 2017-05-03T05:17:35Z

@rhyolight @breznak Done

breznak

Just a minor typo introduced in latest commit, otherwise from me very good! 👍

breznak · 2017-05-03T10:42:25Z

src/nupic/encoders/adaptivescalar.py

+  def __str__(self):
+    string = "AdaptiveScalarEncoder:"
+    string += "  min: {minval}".format(min = self.minval)
+    string += "  max: {maxval}".format(max = self.maxval)


I think these should be minval/maxval

breznak · 2017-05-03T14:58:05Z

@rhyolight can we merge?

rhyolight · 2017-05-03T15:06:41Z

Thanks guys! I can't find anywhere in the codebase dump() was called, except within the RSDE when verbose. This must have been a convention we used when developing encoders. I appreciate the cleanup.

breznak suggested changes Apr 23, 2017

View reviewed changes

abhinavrai44 force-pushed the str branch from e6b5c99 to b6240b4 Compare April 23, 2017 17:47

breznak suggested changes Apr 28, 2017

View reviewed changes

rhyolight suggested changes Apr 28, 2017

View reviewed changes

abhinavrai44 force-pushed the str branch 2 times, most recently from 65a748b to a006f26 Compare May 2, 2017 13:42

breznak suggested changes May 3, 2017

View reviewed changes

Replace dump() with define __str__ in Encoders. Issue numenta#1518

f9aef04

abhinavrai44 force-pushed the str branch from a006f26 to f9aef04 Compare May 3, 2017 10:54

rhyolight approved these changes May 3, 2017

View reviewed changes

breznak approved these changes May 3, 2017

View reviewed changes

rhyolight merged commit 8d97b2b into numenta:master May 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace dump() with define str in Encoders. Issue #1518 #3559

Replace dump() with define str in Encoders. Issue #1518 #3559

abhinavrai44 commented Apr 21, 2017 •

edited by breznak

Loading

abhinavrai44 commented Apr 23, 2017

breznak left a comment

breznak Apr 23, 2017

breznak Apr 23, 2017

breznak Apr 23, 2017

abhinavrai44 commented Apr 23, 2017

abhinavrai44 commented Apr 28, 2017

breznak left a comment

breznak Apr 28, 2017

breznak Apr 28, 2017

breznak Apr 28, 2017

rhyolight Apr 28, 2017

rhyolight Apr 28, 2017

rhyolight Apr 28, 2017

rhyolight Apr 28, 2017

rhyolight Apr 28, 2017

rhyolight Apr 28, 2017

rhyolight Apr 28, 2017

abhinavrai44 commented May 3, 2017

breznak left a comment

breznak May 3, 2017

breznak commented May 3, 2017

rhyolight commented May 3, 2017

		@@ -700,18 +700,8 @@ def closenessScores(self, expValues, actValues, fractional=True):


		def dump(self):

Replace dump() with define __str__ in Encoders. Issue #1518 #3559

Replace dump() with define __str__ in Encoders. Issue #1518 #3559

Conversation

abhinavrai44 commented Apr 21, 2017 • edited by breznak Loading

abhinavrai44 commented Apr 23, 2017

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

abhinavrai44 commented Apr 23, 2017

abhinavrai44 commented Apr 28, 2017

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

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

abhinavrai44 commented May 3, 2017

breznak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breznak commented May 3, 2017

rhyolight commented May 3, 2017

Replace dump() with define str in Encoders. Issue #1518 #3559

Replace dump() with define str in Encoders. Issue #1518 #3559

abhinavrai44 commented Apr 21, 2017 •

edited by breznak

Loading