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

created a separate fft fn for structural array data #39

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

Conversation

asmeyer2012
Copy link

New function deals with the structural array dtypes as inputs to fft_file. The function name is fft_file_complex_structural_array and the syntax is the same.

Instead of assuming the last dimension is size 2 and has the form [real,imag], the function assumes a numpy.dtype type. It extracts the number of dimensions from a while loop,

dt = dset.dtype
dtype_shape = tuple()
while len(dt.shape) > 0:
    dtype_shape += dt.shape
    dt = dt.subdtype[0]
dtype_Ndim = len(dtype_shape)

The bounds checking is modified to take into account the new assumptions. The fft axes are modified to operate on the appropriate dimensions

axes = tuple(-dtype_Ndim-n-1 for n in range(3))
out = get_fft(arr, cuda=cuda, axes=axes)

and same with the slicing

for axis, key in enumerate(["x", "y", "z"]):
    axis = -1 * (axis + 1) -dtype_Ndim
    LOGGER.debug(
        "\t\t Axis %d: %s -> %s[%s]", axis, key, key, slice_index
    )
    out = np.take(out, slice_index, axis=axis)

In addition, the meta attribute is moved outside of the max_momentum if statement to prevent undefined errors happening when max_momentum is None

meta = dset.attrs.get("meta", None)
meta = str(meta) + "&" if meta else ""
if max_momentum is not None:

@asmeyer2012 asmeyer2012 linked an issue Mar 19, 2021 that may be closed by this pull request
@ckoerber
Copy link
Member

ckoerber commented Mar 23, 2021

Hello @asmeyer2012,

Thank you for providing the PR and describing the changes. I have not tested the behavior for this different kind of structural arrays, but from what I can tell, the adjustments make sense to me.

Independent of the structural parts, the meta data adjustments should be implemented for both the fft_file_complex_structural_array and the original method. I would slightly adjust it though to prevent a trailing & char in the scenario where meta is not None but max_momentum is :

meta = dset.attrs.get("meta", "")
if max_momentum is not None:
    meta = str(meta) + "&" if meta else ""

From what I can tell, the proposed change for structural arrays leaves all the core functionality of fft_file in place but changes the underlying assumptions of the dset shape and the variables arr, axes and axis.

I believe now (different compared to the comment in the issue I have made), that it is cleaner to factor this out (including assumptions) in a new infer_fft_array function along the lines of

def infer_fft_array(dset, **kwargs):
   """Computes axes, axis and shape parameters for fft array"""
  # scenario one (old case)
  if dset.shape == ...
    array = dset[()]
    array = (array.T[0] + array.T[1] * 1j).T
    axes = (-1, -2, -3)
    axis = {0: "x", 1: "y", 2: "z"}
  # structural array case
  elif ...:
     dtype_shape = tuple()
     while len(dt.shape) > 0:
         ...
   else:
       raise ValueError("Data set shape and structure did not match expectations. (e.g., ...)"
  return array, axes, axis

This allows to just keep one FFT method simplifying future maintenance

The FFT method would then look like

def fft_file(...):
    ...
     if chunk_size is None:
        arr, axes, axis = infer_fft_array(dset)
    ...

It might be possible to re-use the infer_fft_axes method on other parts.

I am not entirely sure yet if we want to automate decision making by just parsing shapes (what would the elif in infer_fft_array look like?) or if we should make it explicit (i.e., based on a kwarg which is also specified at the fft_file level like dset_kind="structural"?

Also it may make sense to unify the output of infer_fft_array to always return non-structural arrays (so that eventual data is of the same type).

What do you think?

If this makes sense to you, I'd create an update based on your PR.

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.

FFT to work on complex data vs [RE,IM] final shape of data
2 participants