pthom / imgui_bundle

Dear ImGui Bundle: an extensive set of Ready-to-use widgets and libraries, based on ImGui. Start your first app in 5 lines of code, or less. Whether you prefer Python or C++, this pack has your back!
https://pthom.github.io/imgui_bundle/
MIT License
666 stars 66 forks source link

plotting python numpy data that is noncontiguous and using implot stride option don't seem to work #201

Closed west-rynes closed 3 months ago

west-rynes commented 5 months ago

I noticed two potential issues: (or am just missing things)

1) It appears that the wrapping of the implot functions that take numpy arrays expect contiguous data. Would it be possible to enhance the wrapping to handle non-contiguous data. I believe the numpy C api allows that with a flag NPY_ARRAY_C_CONTIGUOUS. This may help users who may not be aware that they need to currently alway supply contiguous data.

2) The stride option in the plot functions does not seem to work as expected.

Sample code illustrating behavior:

import numpy as np
from imgui_bundle import imgui, immapp, implot

x = np.arange(0, np.pi * 5, 0.001)
y = np.sin(x)

def gui():
    if implot.begin_plot("###Plot"):

        implot.plot_line("curve np contiguous array", x, y)

        # Expectation is all lines below should look the same (with slight x offset to see them added) but just downsampled by 2x.

        # plot_line expects contiguous data 
        # Perhaps in the wrapper use np api to request always contiguous data  NPY_ARRAY_C_CONTIGUOUS
        #     https://numpy.org/devdocs/reference/c-api/array.html#c.NPY_ARRAY_C_CONTIGUOUS
        #     can also fix in python with np.ascontiguousarray(...)
        implot.plot_line("curve np sliced array", x[::2]+.1, y[::2]) 
        implot.plot_line("curve np contiguous sliced array", np.ascontiguousarray(x[::2]+.2), np.ascontiguousarray(y[::2])) 

        # Stride does not seem to work as expected. 
        implot.plot_line("curve contiguous with stride", x+.3, y, stride= 2)

        implot.end_plot()
immapp.run(gui, with_implot=True)

Example output

The red line with the stride option mangles the data:

image

The orange curve with a sliced numpy array which then non-contiguous reads incorrectly:

image

Thanks as aways for this great imgui wrapper bundle and the helloimgui ease of use library.

pthom commented 5 months ago

Hello,

As you can see below, the code to transform a numpy array to a full C-Style C Array (which ImPlot expects) is quite complex

https://github.com/pthom/imgui_bundle/blob/5da99660c98aca259f4b6d356ee70e763152d33c/external/implot/bindings/pybind_implot.cpp#L714-L767

Concerning non contiguous arrays: I would advise you to use ascontiguousarray before calling implot, since I think it would be too complex to let the code handle another special case. However, I could raise a readable exception if if the received array is detected as not contiguous.

Concerning the stride option: I'm out of time to study this more in detail now. I'll give it a more complete look next week.

Cheers

west-rynes commented 5 months ago

Since it is a lot of work. Perhaps for other users in the future there would be some info in the documentation to indicate that contiguous should only be used.

That is what I have been doing. I just figured some people may not understand why their plotting is not working so I thought I’d submit an issue.

Don’t worry about the stride option. I just subsample the numpy array and then use the as contiguous numpy function.

I didn’t want to cause any significant work. Thank you for looking at it.

pthom commented 5 months ago

Ok. Two modifications were applied.

  1. check that arrays are contiguous and warn the user if not: https://github.com/pthom/imgui_bundle/commit/e885a8045fbd948344ddb85546fe16f03d662bef
  2. implot bindings: exclude params stride & offset : The way they were implemented depended on the size of the type in C which is not easy to access in Python. It was better to remove support for them altogether. https://github.com/pthom/imgui_bundle/commit/3714d5c65527ac8ba04a89ed3ab309f5d69e202b
Aman-Anas commented 5 months ago

Doesn't the offset param simply depend on index from the user side? I have been able to use the offset param successfully for some plotting widgets, didn't experience any issues with it before. It's very useful for making "scrolling" graphs and such by offsetting an array like a circular buffer.

pthom commented 5 months ago

@Aman-Anas: Thanks for your remark and reactivity!

Indeed offset does not depend on the size of the type as long as stride is not used.

See extract from ImPlot code below:

template <typename T>
IMPLOT_INLINE T IndexData(const T* data, int idx, int count, int offset, int stride) {
    const int s = ((offset == 0) << 0) | ((stride == sizeof(T)) << 1);
    switch (s) {
        case 3 : return data[idx];
        case 2 : return data[(offset + idx) % count];
        case 1 : return *(const T*)(const void*)((const unsigned char*)data + (size_t)((idx) ) * stride);
        case 0 : return *(const T*)(const void*)((const unsigned char*)data + (size_t)((offset + idx) % count) * stride);
        default: return T(0);
    }
}

When stride is disabled, we only fall into case 2 and case 3 (which are OK). Case 0 and 1 do pointer arithmetic that depend on the size (IMHO (const unsigned char*)data + (size_t)((idx) ) * stride is a valid pointer iif stride is a multiple of sizeof(T))`

See commit https://github.com/pthom/imgui_bundle/commit/4316732046e53aabfbd6352bf10e1bd9b142f685 that restores the offset.