Sparse neighbours

[bluescarni]

Here is the initial implementation of the sparse representation of the neighbours.

The voronoi_grid class in voronoi_helpers.py now has an extra attribute, called st (sparse table). This is a tuple of 2 elements: the list of all neighbours of all cells, and a list of indices specifying at what points in the first list the neighbours for the n-th cell start. This second list always has a size of nsites + 1, while the first list has a size that depends on the initial distribution of the sites.

Example: st[0] is [0,2,3,4,6,1,4,5,6,4,3] and st[1] is [0,3,4,11]. This means that:

• cell 0 has neighbours [0,2,3],
• cell 1 has neighbours [4],
• cell 2 has neighbours [6,1,4,5,6,4,3].

The previous dense representation is available as before in the neighbours_table attribute. I have added some checks to the existing test code in order to make sure that the sparse representation is consistent with the dense one.

I also made some optimisations to the initial setup of the tessellation on the Python side (specifically, the check that all sites are within the domain boundaries is now done in numpy and hence much faster).

[bluescarni]

I dropped all the multithreaded-related stuff for now. I need more time to figure out a way to work around the problems on OSX.

[astrofrog]

@bluescarni - thanks for your work on this! Could you look into whether you can figure out how to adapt the code on the Fortran side to take the new format?

[bluescarni]

@astrofrog Sure. This is where I need to do the modifications, if I have understood correctly?

https://github.com/hyperion-rt/hyperion/blob/master/src/grid/grid_geometry_voronoi.f90

[astrofrog]

@bluescarni - yes, that should be the right file to modify. Let me know if you have any questions about Fortran syntax!

[bluescarni]

@astrofrog I have pushed the changes we discussed (I just overrode the current PR with the new branch).

[astrofrog]

For some reason the new version uses more memory. Investigating.

Old

New

[astrofrog]

Apparently this happens in the Fortran code, which is weird.

[bluescarni]

Mhm... I am noticing that, while the sparse representation saves some memory, we do have to store the indices as well. I am wondering if maybe that balances things out.

[astrofrog]

@bluescarni - I don't think that's going to be it, I think there should still see a net gain. This may be related to some other changes btw, the 'old' version above is from a few weeks ago, so checking.

[astrofrog]

Apart from the changes we discussed, I think this is good algorithmically. I ran for many photons and cells and no errors :) So once you resolve the table/dataset issue, we can merge!

[bluescarni]

@astrofrog I submitted the preliminary implementation of the change we discussed, that involves only the neighbours. If it looks good to you I can proceed to make the changes to the rest of the table as well.

[astrofrog]

@bluescarni - this works a lot better now and doesn't show the strange memory behavior - thanks for fixing it! The copying is not much faster but it's much more memory efficient. I don't think we need to extend this to that remaining table since there are no memory issues there.

[astrofrog]

This PR with the latest changes

[astrofrog]

Well actually it is quite a bit faster, I take that back. But I'm not sure if we'll see much gains for the main table since there is no memory issue there.

[astrofrog]

Works great, thanks! :)

[astrofrog]

@bluescarni - just noticed that the np.int32 was removed during debugging - can you open another small PR to add back the dtype=np.int32 so we can check things still pass?

[bluescarni]

@astrofrog Cheers! I will add back the int32 check in another PR. Good to see the speedup and the reduction in memory usage.

If you want I can still push this a bit forward with similar modifications in the table.

[astrofrog]

@bluescarni - for the table: why not, we can give it a try! :)