getkeops / keops

KErnel OPerationS, on CPUs and GPUs, with autodiff and without memory overflows
https://www.kernel-operations.io
MIT License
1.04k stars 64 forks source link

Problem with `elemT` operation #185

Closed AmelieVernay closed 2 years ago

AmelieVernay commented 3 years ago

The doc says that elemT() takes three inputs, but the compiler asks for two (example with rkeops):

formula = "Sum_Reduction(x + ElemT(y, 5, 1), 1)"
args = c("x=Vi(5)", "y=Pm(1)")
op1 <- keops_kernel(formula, args)
In file included from <command-line>:
./headers317c4b98428a8dd80d42edf44.h:27:31: error: macro "ElemT" passed 3 arguments, but takes just 2
   27 | using F = decltype(InvKeopsNS(FORMULA_OBJ));
      |                               ^~~~~~~~~~~
In file included from xxx/keops/rkeops/inst/include/keops/core/formulas/maths/GradMatrix.h:7,
                 from xxx/keops/rkeops/inst/include/keops/keops_includes.h:91,
                 from ./headers317c4b98428a8dd80d42edf44.h:8,
                 from <command-line>:
xxx/keops/rkeops/inst/include/keops/core/formulas/maths/ElemT.h:41: note: macro "ElemT" defined here
   41 | #define ElemT(p,k) KeopsNS<ElemT<decltype(InvKeopsNS(p)),k>>()
      | 
In file included from xxx/keops/rkeops/inst/include/keops/keops_includes.h:131,
                 from ./headers317c4b98428a8dd80d42edf44.h:8,
                 from <command-line>:
xxx/keops/rkeops/inst/include/keops/core/reductions/Sum_Reduction.h:108:72: error: missing template arguments before ‘)’ token
  108 | #define Sum_Reduction(F, I) KeopsNS<Sum_Reduction<decltype(InvKeopsNS(F)),I>>()
      |                                                                        ^
./headers317c4b98428a8dd80d42edf44.h:26:21: note: in expansion of macro ‘Sum_Reduction’
   26 | #define FORMULA_OBJ Sum_Reduction(x + ElemT(y, 5, 1), 1)
      |                     ^~~~~~~~~~~~~
./headers317c4b98428a8dd80d42edf44.h:27:31: note: in expansion of macro ‘FORMULA_OBJ’
   27 | using F = decltype(InvKeopsNS(FORMULA_OBJ));
      |                               ^~~~~~~~~~~
compilation terminated due to -fmax-errors=2.
joanglaunes commented 3 years ago

Hello @AmelieVernay , Thank you for noticing this, I think the commit I just did now in master fixes this issue ; could you check it ?

AmelieVernay commented 3 years ago

Thank you! It works now.

P.S. - It is not that important, but looking at the associated operations elem, extract, and extractT, I think that the order of the input arguments in elemT is counterintuitive: the M argument corresponding to the position is the second argument in the three above-mentioned operations, but is the last one in elemT.

I hope I'm clear enough.

joanglaunes commented 2 years ago

Hello @AmelieVernay , I agree it would have been better to put the M argument in second position for elemT also, anyway as you say it is not that important and it would break compatibility if we do it now. I am closing this issue now since it is fixed.