manodeep / Corrfunc

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

pair.dx, dy, dz not available for DDrppi? #141

Closed qx211 closed 6 years ago

qx211 commented 6 years ago

Dear sir,

I have adopted the custom weighting function procedure and done good tests with DD and DR, including the use of pair->dx, dy, dz.

However, when I tried to use dx, dy for the projected pairs, it always returns segmentation fault, despite successes in using weight_type=pair_product.

Could you please be able to shed some light on this issue?

Thanks, Qianli

lgarrison commented 6 years ago

Sure, we'll be happy to take a look. Are you using theory.DDrppi or mocks.DDrppi?

Could you post your custom weighting function? Also, remember that pair.dx is of type weight_union_DOUBLE, so your weighting function must access pair.dx.d, pair.dx.s, or pair.dx.a, depending on which instruction set you're using.

qx211 commented 6 years ago

Thanks, so I was using DDrppi from: from Corrfunc.mocks.DDrppi_mocks import DDrppi_mocks

the custom weighting function I was using is: (showing only the DOUBLE one, AVX and SSE follows similarly with dx.a or dx.s specified etc.)

static inline DOUBLE MY_GAMMA_T_DOUBLE(const pair_struct_DOUBLE *pair){
    DOUBLE CTWOPHI;
    DOUBLE STWOPHI;
    CTWOPHI = -1.0 * ((pair->dx.d*pair->dx.d) - (pair->dy.d*pair->dy.d))/
              ((pair->dx.d*pair->dx.d) + (pair->dy.d*pair->dy.d));
    STWOPHI = -1.0 * (2.0*pair->dx.d*pair->dy.d)/
              ((pair->dx.d*pair->dx.d) + (pair->dy.d*pair->dy.d));

    return  CTWOPHI*pair->weights1[0].d + STWOPHI*pair->weights1[1].d;
}
lgarrison commented 6 years ago

I don't immediately see any problem with this function. Does the crash still happen if you use the 'fallback' kernel?

We'll probably need example code that produces the crash to debug further. You may wish to fork the repository so we can download and compile the code with your custom weight function.

qx211 commented 6 years ago

The problem doesn't occur when I used DD for 3D pairs though. I would definitely take your suggestion by forking the repository. But I haven't used the "fallback" kernel before, do you know how can I enable it?

Best wishes,

lgarrison commented 6 years ago

Pass the argument isa='fallback' to the DDrppi_mocks function. Similarly, you can use 'avx' and 'sse' to test the other kernels.

On Wed, Nov 1, 2017 at 3:09 PM, qx211 notifications@github.com wrote:

The problem doesn't occur when I used DD for 3D pairs though. I would definitely take your suggestion by forking the repository. But I haven't used the "fallback" kernel before, do you know how can I enable it?

Best wishes,

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/manodeep/Corrfunc/issues/141#issuecomment-341208564, or mute the thread https://github.com/notifications/unsubscribe-auth/AFYPy5txgqOoWk_rwKhGG-m83sEEijwCks5syMHtgaJpZM4QOizs .

qx211 commented 6 years ago

Thanks! I will let you know what comes out!

manodeep commented 6 years ago

@lgarrison Could this be related to #110 ?

@qx211 How did you install Corrfunc? via git clone or pip install

manodeep commented 6 years ago

@qx211 Side-note -- you should get better performance if you rewrite the function in the following (untested) manner:

static inline DOUBLE MY_GAMMA_T_DOUBLE(const pair_struct_DOUBLE *pair){
    DOUBLE CTWOPHI;
    DOUBLE STWOPHI;
    const DOUBLE dx = pair->dx.d;
    const DOUBLE dy = pair->dy.d;
    const DOUBLE sqr_dx = dx * dx, sqr_dy = dy * dy;
    const DOUBLE inv_sqr_dx_p_sqr_dy = 1/(sqr_dx + sqr_dy);
    CTWOPHI = (sqr_dy - sqr_dx) * inv_sqr_dx_p_sqr_dy;
    STWOPHI = -2 * dx * dy * inv_sqr_dx_p_sqr_dy;

    return  CTWOPHI*pair->weights1[0].d + STWOPHI*pair->weights1[1].d;
}

Re-writing this way reduces the number of computations, keeps all the calculations in the same precision (float or double), and removes one (expensive) division operation.

manodeep commented 6 years ago

@qx211 Did you get a chance to look at this?

manodeep commented 6 years ago

@qx211 I am going to close this issue, please feel free to re-open if you had any trouble getting the code to work with the fallback kernel.