clEsperanto / pyclesperanto

GPU-accelerated Image Processing library using OpenCL
https://clesperanto.github.io/
BSD 3-Clause "New" or "Revised" License
31 stars 6 forks source link

Rename parameters #260

Open haesleinhuepf opened 1 week ago

haesleinhuepf commented 1 week ago

Hi @StRigaud ,

I'm just elaborating how many parameter names have been renamed between pyclesperanto_prototype and pyclesperanto. It's a very long list. If I wanted to rename a parameter, where would be a good place to do this?

I'm a bit confused, because according to the pyclesperanto CLIc wrapper lists parameter names "src0" and "src1" for add_imagesweighted [here](https://github.com/clEsperanto/pyclesperanto/blob/a2dd16cb3fe9947000a40f6a8432e7d1d6f915b1/src/wrapper/tier1.cpp#L13C5-L15C114). But in the python code, its "input_image0" and "input_image1". Is this mapping "src0" -> "input_image0" done automatically somewhere?

Thanks!

Cheers, Robert

StRigaud commented 1 week ago

If I wanted to rename a parameter, where would be a good place to do this?

In the C++ code and doxygen blocks! then it is propagated to the python and java wrapper. Same for anything that touch generated code.

its "input_image0" and "input_image1". Is this mapping "src0" -> "input_image0" done automatically somewhere?

In the C++ i am always using src and dst but for python you where using input_image or output_image (and sometime other names) hence, for these particular parameters I automatically replace src by input_image and dst by output_image in the gencle when generating the python code which call the wrapper code. The wrapper code stick to the C++ names.

haesleinhuepf commented 1 week ago

What if we needed to implement some function-specific naming? E.g. the function generate_distance_matrix consumes to coordinate lists, but these parameters are now called "input_image0" and "input_image1", which might be confusing. Where would I specify better names for these parameters?

StRigaud commented 1 week ago

Quickly, no better options than doing a dirty function only in the python side (interoperability.py)

On the long run, re-work the autogenerated logic and c++ code. Which I am ok to do but after finalizing the Java/paper/etc.

Input and outputs are specifically handled, so difficult to change.

haesleinhuepf commented 1 week ago

Ok, I'm adding here a list of parameters that I think are inappropriate or not reasonable. The notebook which generated this list is here.

Example: The old names (right, _prototype) were self-explanatory, the new names (left) not really:

My selection:

I'm afraid end-users may struggle with these kind of changes, especially as they are so many.

There are also many functions with changes of this kind:

StRigaud commented 1 week ago

To be solved later with re-work of the auto-generator and input/output special case management

Numerical parameter name can be changed in CLIc directly.

StRigaud commented 1 week ago

after auick checkup, I think we can change the input / output parameter name without too much issue. Still need to check. I would, however, try to stick to input_image and output_image as much as possible for consistancy, and wait for after the workshop

haesleinhuepf commented 1 week ago

How about input_coordinates or input_labels or input_binary_image here and there? Or after I2K?

StRigaud commented 1 week ago

I will try to generate the python code to check for issue

This modification should still be done in the CLIC side. I would start with only the input and deal with output later :)

if you wanna start now you can, I can test if no issue when generating but if some problem raise I will side it for after I2K

StRigaud commented 1 week ago

Ok after a quick check, simply changing the input name seems ok with the auto-generator (It seems I was not too stupid when I code it lol).

If you want to change some name @haesleinhuepf go for it but I still push for some caution: focus on the important changes and not too much (we never know)

haesleinhuepf commented 1 week ago

Ok, I'm going to change the most important image parameter names in a separate PR. You can then test it before merging it. And if you merge it after i2k, that's ok, too.