random -> numpy.random #146

tcld · 2015-10-23T15:13:12Z

Here the replacement of all uses of the random-module with respective numpy-methods.
Since this is kind of a break with save-compatibility and...everything, I guess, I also took the freedom to include some small changes so they aren't necessary in the future - I will add comments to the code to point them out and explain them.

Notes:

This is based on Make use of Numpy-Arrays #138 .
This does not pass the ancient-map tests (since that one uses RNG to determine its outcome)

tcld · 2015-10-23T15:16:26Z

tests/basic_map_operations_test.py

@@ -9,14 +9,13 @@ def setUp(self):
        pass

    def test_random_point(self):
-        for seed in [0, 1, 27, 29, 1939, 1982, 2015]:
-            random.seed(seed)


This was very much unnecessary unless the RNG is expected to be of very dubious quality. I chose to simplify it.

tcld · 2015-10-26T11:16:13Z

I might change some small things about this before uploading the new blessed ancient maps. Once that is done the tests should be fine again.

tcld · 2015-10-26T18:35:50Z

It would be nice if somebody could look over the code; I think I touched all that is necessary, but this will need some polish, I presume.

tcld · 2015-10-26T18:36:54Z

tests/basic_map_operations_test.py

@@ -8,16 +8,6 @@ class TestBasicMapOperations(unittest.TestCase):
    def setUp(self):
        pass

-    def test_random_point(self):


The method random_point() isn't used anymore so I removed it (and this test). It was an almost trivial method anyway.

ftomassetti · 2015-10-26T19:18:53Z

It looks good to me

tcld · 2015-10-26T21:10:43Z

worldengine/generation.py


    if not step.include_precipitations:
        return w

+    # Prepare sufficient seeds for the different steps of the generation
+    rng = numpy.random.RandomState(w.seed)  # create a fresh RNG in case the global RNG is compromised (i.e. has been queried an indefinite amount of times before generate_world() was called)
+    sub_seeds = rng.randint(0, 4294967295, size=100)  # sys.maxsize didn't quite work


I don't like the use of this fixed number at all. Does somebody have a better idea? sys.maxint (or whatever it is called) is not available for Python 3; sys.maxsize returns a number that is not compatible with numpy (which takes a uint32).

tcld · 2015-10-26T22:36:03Z

After all this I ended up here:

It might not be immediately visible but everything except the mountains is the same (see the middle island). Before the changes to the RNG there were a lot more (small) differences. Does anybody have an idea why the mountains differ?

PS: I am referring to differences between Python 2 and 3.

ftomassetti · 2015-10-26T22:38:30Z

the mountain generation is quite complicate. We have a mask to avoid overlapping mountains and a system to determine the size of each single mountain which is based on the elevation in the area. I am not sure if it is using the world seed.

ftomassetti · 2015-10-26T22:42:09Z

it seems we are not using any seed for the mountains BUT we are using it for the other masks. Basically we assign each pixel to some masks, ~one for biome, and then we draw that pixel accordingly. If any mask change it could affect how mountains are drawn

tcld · 2015-10-26T22:42:35Z

I assume you are talking about this:

def _draw_a_mountain(pixels, x, y, w=3, h=3):
    # mcl = (0, 0, 0, 255)  # TODO: No longer used?
    # mcll = (128, 128, 128, 255)
    mcr = (75, 75, 75, 255)
    # left edge
    for mody in range(-h, h + 1):
        bottomness = (float(mody + h) / 2.0) / w
        leftborder = int(bottomness * w)
        darkarea = int(bottomness * w / 2)
        lightarea = int(bottomness * w / 2)
        for itx in range(darkarea, leftborder + 1):
            pixels[x - itx, y + mody] = gradient(itx, darkarea, leftborder,
                                                 (0, 0, 0), (64, 64, 64))
        for itx in range(-darkarea, lightarea + 1):
            pixels[x + itx, y + mody] = gradient(itx, -darkarea, lightarea,
                                                 (64, 64, 64), (128, 128, 128))
        for itx in range(lightarea, leftborder):
            pixels[x + itx, y + mody] = (181, 166, 127, 255)  # land_color
    # right edge
    for mody in range(-h, h + 1):
        bottomness = (float(mody + h) / 2.0) / w
        modx = int(bottomness * w)
        pixels[x + modx, y + mody] = mcr

Yes, this is complicated.^^ I worked through it a couple of days ago, but there is no directly visible use of something random (and the RNG in draw_ancientmap() is already replaced with a numpy one).

tcld · 2015-10-26T22:49:03Z

I will take another look at it tomorrow. We are very close to the goal already.^^

tcld · 2015-10-26T22:55:41Z

I did one more thing:

So the mountains are clearly different; but I think the background-colors might be slightly shifted, too. (Maybe I just misunderstood how ImageMagick's compare works).
Anyway, it's a nice comparison.

… before. Made small changes to seeding etc.

tcld · 2015-10-28T16:38:20Z

I think I found the cause for the remaining problems:

def _mask(world, predicate, factor):
    _mask = [[False for x in range(factor * world.width)] for y in
             range(factor * world.height)]
    for y in range(factor * world.height):
        for x in range(factor * world.width):
            xf = int(x / factor)
            yf = int(y / factor)
            if predicate((xf, yf)):
                v = len(
                    world.tiles_around((xf, yf), radius=1,
                                       predicate=predicate))
                if v > 5:
                    _mask[y][x] = v
    return _mask

Apparently this map only stores integers in Python 2 (???) but floats in Python 3. It was going to be replaced by numpy anyway, I hope that will fix things.

EDIT: Maybe Python 2 does not automatically typecast at this position?

_mask[y][x] = v / 4   # v is an integer

EDIT2: That last assumption seems to be right. I will just add a typecast, assuming that was the intended behaviour. If not, we can change that later.

Python 2.7.6 (default, Jun 22 2015, 17:58:13) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> 5/4
1

Python 3.4.3 (default, Oct 14 2015, 20:28:29) 
[GCC 4.8.4] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 5/4
1.25

tcld · 2015-10-28T17:28:51Z

I could use some help with the pyflakes- and manifest-tests since I don't think my changes make them fail.

Mindwerks/worldengine-data#5 will take care of the problems with ancient maps.

Other than the failing tests, this code is ready and could be reviewed, if anybody has the time.

tcld · 2015-10-28T17:32:53Z

worldengine/drawing_functions.py

@@ -142,7 +140,7 @@ def _find_mountains_mask(world, factor):
                                           radius=3,
                                           predicate=world.is_mountain))
                if v > 32:
-                    _mask[y][x] = v / 4
+                    _mask[y, x] = v / 4.0  # force conversion to float, Python 2 will *not* do it automatically


Not sure which behaviour was the intended one here. The ancient map output looks different, I think the mountains are spread a little thinner for the old behaviour int(v/4).

ftomassetti · 2015-10-28T18:42:49Z

Restarting tests after the changes to worldengine data

tcld · 2015-10-28T18:55:30Z

I consider this passed.^^ It would be nice if this could be the next merge so I can cross this off my list and move on to finishing another branch (instead of having to come back here and rebase/fix things).

ftomassetti · 2015-10-28T19:30:50Z

worldengine/basic_map_operations.py

-import random
-
-
-def random_point(width, height):


I think it could be nice to have methods such as random_point or perhaps random_land_point but we could add it later if needed

It was only used once. Removing it allowed for another numpy-optimization; so for now this function would have been dead code (and since it is fairly simple anyway, rewriting it would be easy).

EDIT: I am talking about this optimization:
https://github.com/Mindwerks/worldengine/pull/146/files#diff-c341fcae944ab9ce9e38ea34a0165badR337

It is a very important one in my opinion.

ftomassetti · 2015-10-28T19:38:08Z

Looks good to me. I would just leave a possibility to @psi29a to comment on it before merging but it is agreed that this one should be merged soon.

A lot of great work on this one, thanks a lot!

tcld · 2015-10-28T20:04:29Z

I hope I took proper care of your comments.

ftomassetti · 2015-10-28T20:26:11Z

I am satisfied, as I said I would just leave @psi29a the time to comment, if he wishes to do so. If not this is good to go for me.

ftomassetti · 2015-10-30T07:04:58Z

Let's start merging stuff

random -> numpy.random

psi29a · 2015-10-30T07:28:54Z

No need to wait on me... I'm in the middle of moving so I'm MIA for a bit.

tcld reviewed Oct 23, 2015
View reviewed changes

tcld mentioned this pull request Oct 23, 2015

random module gives varying output for different Python versions #145

Closed

tcld force-pushed the rng_refine branch from f72b022 to 6e167db Compare October 26, 2015 11:11

tcld mentioned this pull request Oct 26, 2015

Release new version #139

Open

tcld reviewed Oct 26, 2015
View reviewed changes

tcld force-pushed the rng_refine branch from 3e0047b to d51dd69 Compare October 26, 2015 21:22

tcld added 4 commits October 28, 2015 17:08

Replaced numpys random-module wherever Pythons random-module was used…

48b43fb

… before. Made small changes to seeding etc.

Changed range of possible seeds for worldengine.

11f3f5f

A few more refinements.

1e9861b

Changes according to suggestions.

1f02115

Fixed "randomness" of mountain placement.

2b07689

tcld force-pushed the rng_refine branch from d51dd69 to 2b07689 Compare October 28, 2015 17:12

This was referenced Oct 28, 2015

Added new scatter plot #157

Merged

random -> numpy.random: updated ancient maps Mindwerks/worldengine-data#5

Merged

tcld reviewed Oct 28, 2015
View reviewed changes

ftomassetti reviewed Oct 28, 2015
View reviewed changes

More polishing after suggestions by @ftomassetti.

40e5176

This was referenced Oct 29, 2015

Rework of GDAL-export #159

Merged

numpy-replacements for image-drawing #161

Merged

ftomassetti added a commit that referenced this pull request Oct 30, 2015

Merge pull request #146 from tcld/rng_refine

56e8dba

random -> numpy.random

ftomassetti merged commit 56e8dba into Mindwerks:master Oct 30, 2015

tcld deleted the rng_refine branch October 30, 2015 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

random -> numpy.random #146

random -> numpy.random #146

tcld commented Oct 23, 2015

tcld Oct 23, 2015

tcld commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld Oct 26, 2015

ftomassetti commented Oct 26, 2015

tcld Oct 26, 2015

tcld commented Oct 26, 2015

ftomassetti commented Oct 26, 2015

ftomassetti commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld commented Oct 28, 2015

tcld commented Oct 28, 2015

tcld Oct 28, 2015

ftomassetti commented Oct 28, 2015

tcld commented Oct 28, 2015

ftomassetti Oct 28, 2015

tcld Oct 28, 2015

ftomassetti commented Oct 28, 2015

tcld commented Oct 28, 2015

ftomassetti commented Oct 28, 2015

ftomassetti commented Oct 30, 2015

psi29a commented Oct 30, 2015

random -> numpy.random #146

random -> numpy.random #146

Conversation

tcld commented Oct 23, 2015

tcld Oct 23, 2015

Choose a reason for hiding this comment

tcld commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld Oct 26, 2015

Choose a reason for hiding this comment

ftomassetti commented Oct 26, 2015

tcld Oct 26, 2015

Choose a reason for hiding this comment

tcld commented Oct 26, 2015

ftomassetti commented Oct 26, 2015

ftomassetti commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld commented Oct 26, 2015

tcld commented Oct 28, 2015

tcld commented Oct 28, 2015

tcld Oct 28, 2015

Choose a reason for hiding this comment

ftomassetti commented Oct 28, 2015

tcld commented Oct 28, 2015

ftomassetti Oct 28, 2015

Choose a reason for hiding this comment

tcld Oct 28, 2015

Choose a reason for hiding this comment

ftomassetti commented Oct 28, 2015

tcld commented Oct 28, 2015

ftomassetti commented Oct 28, 2015

ftomassetti commented Oct 30, 2015

psi29a commented Oct 30, 2015