search

manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

Box size for each dimension in Corrfunc.theory #247

Closed johannesulf closed 1 year ago

johannesulf commented 3 years ago

Thanks everyone for this great package. I have a question/request and I hope I didn't just miss an explanation in the documentation. Currently, pair counter functions in Corrfunc.theory only accept a single number for the box size, i.e. one assumes the simulation volume to be a cube. But the functions can all apparently deal with non-cubic boundary conditions if boxsize is not specified. In this case, Corrfunc tries to infer the box size based on the maximum difference in each dimension. In practice, this always ends up with different lengths in each dimension.

So can the user specify non-cubic box sizes and if not, would it be possible to add this feature? Based on the fact that Corrfunc can apparently deal with such a case, it only seems an issue of allowing the user to specify the length in each dimension instead of one length for all dimensions.

I think this would be a very useful feature. Non-cubic boundary conditions are common when one tries to simulate the AP effect since the line-of-sight (z-axis) and perpendicular directions (x and y-axes) are apparently stretched by different factors. So even if the original simulation has a cubic volume, after simulating the AP effect, this isn't the case anymore.

lgarrison commented 3 years ago

That makes sense, I think such a feature would be useful. You're right that the internal mechanisms are there, it's just a matter of plumbing the functionality to take a boxsize tuple from the Python layer to the C layer. I think we would just have the C layer always take a boxsize tuple, but that may also require changes to the options struct, e.g. https://github.com/manodeep/Corrfunc/blob/master/utils/defs.h#L63. @manodeep, what do you think?

manodeep commented 3 years ago

Yup - should be do-able. Think we should let the python wrappers be flexible to take in a single number or a set of 3 numbers for each of the 3 axes. I am not confident about the C-API though. @lgarrison You might have more insight since you already started similar work in #199

Changing the C-struct should be easy enough - maybe something like adding a double box_extents[3], or adding a union (thereby saving 8 bytes)


union {
    double boxsize;
    double extents[3];
}

We might still have space in the header struct such that the two/three additional doubles do not break the ABI.