Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FFTPower to only use positive mu by default #629

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akrolewski
Copy link
Contributor

@akrolewski akrolewski commented Aug 5, 2020

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@rainwoodman
Copy link
Member

rainwoodman commented Oct 14, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants