pec27 / hfof

Friends-of-Friends via spatial hashing
MIT License
15 stars 3 forks source link

error fof 3D variable n (num_orig) no defined #5

Open jegarfa opened 1 year ago

jegarfa commented 1 year ago

Hi, I am runing hfof for a 3D dataset. However, I got an error related to the variable num_orig (Number of points that are not 'false' images):

Traceback (most recent call last):
  File "/home/user/hfof/test3_readbin.py", line 18, in <module>
    fof_labels = fof(pos, r_cut, boxsize=0.015258789) # integer labels from 0...
  File "/user/anaconda/envs/yt/lib/python3.8/hfof-0.2.0-py3.8-linux-x86_64.egg/hfof/cluster.py", line 79, in fof
    domains = fof3d_periodic(cells, N, M, n, old_idx, rcut, sort_idx, pos, log=log)
NameError: name 'n' is not defined

In, fact I checked out the source code (line 79 of cluster.py) and the variable n is not defined. Do you know what could be the proper definition of this variable?

pec27 commented 1 year ago

Ah that's a bug, though probably the reason it's not tested is that it's quite a large linking length for the box size (more than 1/4 of the box width) which means it has to treat periodicity differently. 

hfof uses an absolute linking length and numbers in the astronomy literature often quote as relative (e.g. b=0.2). Is it possible you've used a relative linking length? 

jegarfa commented 1 year ago

That's right, the issue is related to the value of the linking length, I was using the same 'rcut' value than the example. I was also scaling the positions and the lengh of the box by a factor 1.0/65536 as in the example. The original box has L=1000Mpc/h but I am passing L=0.015258789. Is it convenient to scale the units or better to work with the original ones?

I am using now a absolute linking length computed as the 20% of the mean interparticle separation, but I got Segmentation fault (core dumped). Probably this is now related to memory handling.

pec27 commented 1 year ago

It should be fine to use original units. For the segfault, was there any log output beforehand? (You can call fof with log=sys.stdout). This is just a guess, but if the positions are not a C-contiguous (N,3) array then I think they will be copied, which for large numbers of points might cause a segfault.

pec27 commented 1 year ago

I pushed a fix for the large linking length, and fixed up an off-by-one error there that could cause a segmentation fault. Could you retry and let me know if you still get the error?

jegarfa commented 1 year ago

Hi, thank you for the update.

I tried the new version with log=sys.stdout and still got a Segmentation fault. I suspect it is because of the large number of particles I am using, (1024^3, 3), although the machine I am using has enough memory to allocate this amount. Using smaller simulations I did not get errors.

This is the entire output:

Padding positions
Loading libhfof - C functions for FoF calculations lib/python3.8/site-packages/hfof-0.2.0-py3.8-linux-x86_64.egg/hfof/../build/libhfof.cpython-38-x86_64-linux-gnu.so
0 images (c.f. guessed=5,033,165)
Finding primes
Searched 3 + 6 composites
N=     4441 (prime index conversion factor) 0x1159
M= 19722487 (prime index conversion factor) 0x12cf0f7
Inserted 0 images
rcut 9.765625e-05
Finding cells
Sorting cells
3d fof
Segmentation fault (core dumped)
pec27 commented 1 year ago

Sorry for the slow response. If your positions are in float32 then they will be cast to float64, which for 1024^3 positions means hfof will try to allocate 24GB, which is a limitation of the current version of hfof.

However from your log this step seems to have been successful, so I think you've found a bug in the linking itself, although I haven't managed to reproduce it yet.

Interestingly there were no images added, i.e. no particles were near enough to the sides of the box to be linked to another box (I'm guessing this is some kind of zoom simulation?). This implies the periodic FoF is equivalent to the non-periodic one, so you could try that (i.e. leave boxsize as None)