getkeops / keops

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

LogSumExpWeight reduction not available #344

Closed gdurif closed 8 months ago

gdurif commented 8 months ago

Hi,

Per Keops documentation, LogSumExpWeight reduction should be available in Python binding.

Following the example for SumSoftMaxWeight in the doc, I tried to use LogSumExpWeight in the following code:

import numpy as np
from pykeops.numpy import Genred

formula = "Sum(V0-V1)"
variables = ["V0=Vi(3)", "V1=Vj(3)", "V2=Vj(3)"]

# Compile corresponding operator
op = Genred(formula, variables, reduction_op="LogSumExpWeight", axis=0, formula2 = "V2")

# data
x = np.random.randn(4, 3)
y = np.random.randn(5, 3)
w = np.ones([5,3])

# Perform reduction
res = op(x, y, w, backend="CPU")

And I get the following error:

NameError: name 'LogSumExpWeight_Reduction' is not defined

Note 1: I have no problem when using LogSumExp reduction (without weights) or SumSoftMaxWeight (with weights).

Note 2: I stumble across this issue when fixing remaining issues in RKeOps v2+ before submitting it to CRAN.

joanglaunes commented 8 months ago

Hi @gdurif , Thank you for noticing this. I just fixed this now ; in fact there was a mismatch between the convention used in the documentation and what we decided in the code. In the code, it was simply LogSumExp for both cases : with or without weights, depending on wether the formula2 kwarg is provided. So now I have added the possibility to use LogSumExpWeight name, to match the documentation. So when a formula2 kwarg is given, you can use either LogSumExp or LogSumExpWeight. Note that if you want to merge the current main branch to your rkeops branch, which I recommend because there have been several bug fixes, you might encounter one issue because I decided last week to switch from int to signed long int types for many variables in the c++ code, and this may affect the interface with the R binding. Just let me know if it breaks things in your case.

gdurif commented 8 months ago

Hi @joanglaunes Cool, thanks! Regarding your note, since we now use directly pykeops in rkeops, I think that this change will have no effect but I will merge the main branch just in case 👍