HPCE / hpce-2016-cw5

0 stars 2 forks source link

Confusion in ising_spin init() #29

Closed jeremych1000 closed 7 years ago

jeremych1000 commented 7 years ago

https://github.com/HPCE/hpce-2016-cw5/blob/master/include/puzzler/puzzles/ising_spin.hpp#L129-L131

for(unsigned x=0; x<n; x++){
        for(unsigned y=0; y<n; y++){
          out[y*n+x] = (seed < 0x80001000ul) ? +1 : -1;
          <...>

What's the reason of looping columns then rows, but assigning rows then columns? I'm a bit confused.

m8pple commented 7 years ago

It's the kind of thing you see when domain experts are taking short-cuts, as they are implementing models that they know will have the algorithmic properties they need, but it is not necessarily "correct".

In this case, as the "domain expert", my interest is in the statistical properties of the simulation, and I know that:

This means I don't really care whether the rows and columns are flipped, as it still has the same application-level result (a grid of biased random flips).

So as the creator of the code, I know that the exact order that it the random numbers are mapped to the grid doesn't matter. Once the code is working, even if I notice this "bug", I won't correct it, as I've already got results based on this initialisation order.

When I create these things, I deliberately leave stuff like that in, as it is realistic - whether you are given code by somebody else, or you are trying to make your own code faster, you often have to adhere to the slightly strange behaviour of the code. Backwards compatibility of results and the ability to check your sequential implementation against the parallel implementation often outweigh the desire to clean up code. Usually clean up is left to the next major revision (or never).

jeremych1000 commented 7 years ago

Would it be wise to modify the loops in user_ising_spin? Currently looping columns then rows will mean cache-misses all the time, right?

m8pple commented 7 years ago

Yes, that access pattern would have worse cache behaviour, especially for larger grids. So if you can flip the mapping of points to array locations round, without affecting the output, then that could be a good idea.