search

bccp / nbodykit

Analysis kit for large-scale structure datasets, the massively parallel way
http://nbodykit.rtfd.io
GNU General Public License v3.0
111 stars 60 forks source link

Fix FFTPower to only use positive mu by default #629

Open akrolewski opened 4 years ago

akrolewski commented 4 years ago

As pointed out by @grantmerz, FFTPower now bins mu between -1 and 1. However, often it will be passed fields that are only defined for positive mu values, and this creates unexpected behavior and nans. This PR changes FFTPower to use positive mu by default (as it did before #610), and to allow the user to request it to use negative mu bins if they desire.

rainwoodman commented 4 years ago

I am a bit surprised that this doesn't break any test cases.

Could you or @grantmerz elaborate the unexpected behaviour for record? Is it possible that we can fix it without adding a flag? For example, special case if the type of the field is compressed?

akrolewski commented 4 years ago

The following code leads to the unexpected behavior (generating the mesh with ArrayMesh and then using FFTPower to get 2D P(k)). The mesh is real, so its Fourier transform has no negative mu values, but FFTPower histograms mu between -1 and 1 and thus spits out all nans for the first 5 bins in mu. The mesh is still real even if I pass dtype='c16' to ArrayMesh.

initname = '/global/cscratch1/sd/g4merz/recon/den0512.bin' init_dat = np.fromfile(initname,dtype=np.float32,sep='') arr_init = init_dat.reshape((N,N,N),order='F') mesh_init = ArrayMesh(arr_init,BoxSize=1000) r_cross = FFTPower(mesh_init, mode='2d', Nmesh=512, Nmu=10, dk=0.005, los=[0,0,1])

Yes, checking whether the field is compressed should work--I'll look into it.

rainwoodman commented 4 years ago

Thanks for the explanation.

This is probably the compressed attribute to look for: https://github.com/rainwoodman/pmesh/blob/70da89e729911e6c2ae9e56fb2a4e8d41b34b46f/pmesh/tests/test_pm.py#L274

Looks like when the field is compressed we also could use the symmetry to fill the negative mu bins?

akrolewski commented 2 years ago

Hi @rainwoodman , is this pull request able to be merged into master or are there additional checks needed?

rainwoodman commented 2 years ago

I think we agreed use_negative_mu and whether the field is compressed are somewhat degenerate. Perhaps add a comment in the docstring that use_negative_mu=True usually shall be paired when the corresponding real field type is c16 or c8, if you still have the branch?