Fix spline interpolation segfaults. #309

NikoOinonen · 2024-10-07T14:53:39Z

Fixes #308
Fixes #279

This PR moves the spline parameters in the TIP namespace into a struct in the C++ code and introduces a corresponding Python class for managing the parameter arrays. A pointer to the parameters are now explicitly passed into the relevant functions instead of being set on a global level. This gets rid of the problem where the parameter array pointer could still be set even after the arrays were freed.

Also fixes one uninitialized variable in the spline interpolation function that was sometimes segfaulting.

All tests are now enabled and seem to be passing without problems again.

This seems to fix the segfault when running tests with pytest.

…thon interface.

NikoOinonen · 2024-10-07T14:55:52Z

@ProkopHapala The way I did it is maybe not exactly what you were suggesting, but I think this should work quite nicely. It's now more explicit when the spline interpolation is used. The downside is of course that there is more boilerplate passing of the parameters in several functions, but maybe it's acceptable. See if you like it or not.

ProkopHapala

OK

ProkopHapala · 2024-10-07T17:57:26Z

ppafm/core.py

+    def __init__(self, xs, ydys):
+        super().__init__()
+        self.rff_n = len(xs)
+        self.rff_xs = np.ctypeslib.as_ctypes(xs.astype(np.double))


so yo store just the ctypes.pointer, not numpy array? Does this prevent GC to deallocate the memory? Anyway, I would perhaps store the numpy array for debugging purposes (so that we can easily plot the spline, for example)

I originally tried to do it with the np.ctypeslib.ndpointer so that you could just use the numpy array, but for some reason that does not work with structs, so I opted to use the ctypes pointer instead which works.

I changed it so that the original numpy arrays are stored there as well in a separate variables starting with np_.

ProkopHapala · 2024-10-07T17:58:38Z

ppafm/HighLevel.py

+    FFboltz=None,
+    FFkpfm_t0sV=None,
+    FFkpfm_tVs0=None,
+    spline_parameters=None,


I think name tip_spline is better than spline_params, because it tell more what is the purpose of the spline. But that is just cosmetic, perhaps does not matter to much

I changed the name to tip_spline.

ProkopHapala · 2024-10-07T18:03:57Z

tests/human_eye/test_TipForce.py

+
+    parameters = common.PpafmParameters()
+
+    print("TipRSpline.ini overrides harmonic tip")


hmm, so the user should do this manually? I think it would be nice if the spline-class can initialized itself form spline_ini file, just for convenience

No, actually I also made it so that you can load it directly from a file using core.SplineParameters.from_file. This was already used in relaxed_scan.py. I now changed it in this test script as well.

ProkopHapala · 2024-10-07T18:05:07Z

tests/human_eye/test_TipForce.py

+    print("ydys: ", ydys)
+    plt.plot(xs, ydys[:, 0], "o")
+    plt.plot(xs, ydys[:, 0] + ydys[:, 1], ".")
+    spline_parameters = core.SplineParameters(xs, ydys)


I mean it would be convenient to call core.SplineParameters(fname='TipRSpline.ini') instead

I mean I would rather do this

tip_spline = core.SplineParameters( fname='TipRSpline.ini' ) plt.plot(tip_spline.xs, tip_spline.ydys[:,0], "o") plt.plot(tip_spline.xs, tip_spline.ydys[:,0] + tip_spline.ydys[:,1], ".")

Now it's like this.

ProkopHapala · 2024-10-07T18:07:08Z

tests/human_eye/test_vdwDamping.py

this is just some formating? I don't see relation to spline (?)

Yes, it's just moving the code inside a function block, which makes it work better with the automated test system.

ProkopHapala · 2024-10-07T18:17:54Z

@ProkopHapala The way I did it is maybe not exactly what you were suggesting, but I think this should work quite nicely. It's now more explicit when the spline interpolation is used. The downside is of course that there is more boilerplate passing of the parameters in several functions, but maybe it's acceptable. See if you like it or not.

OK, if you want why not.

just to give some background / explanation. This spline-thing was just quick hack which somebody (Adam Sweetman ?) asked me at one conference. It has very limited niche. Therefore I wanted to implement it with minimal effort and minimal changes to the rest of the code.

Usually I try to avoid changes to function interfaces especially on the python/C++ interface, and keep the argument list short.

DLLEXPORT int relaxTipStrokes_omp( int nx, int ny, int probeStart, int relaxAlg, int nstep, double * rTips_, double * rs_, double * fs_, TIP::SplineParams *splineParams ){

On the other hand I agree that it was introducing some hidden variable which was not obvisous/explicit. So perhaps it is better this way.

…nged variable name.

NikoOinonen · 2024-10-08T06:22:15Z

On the other hand I agree that it was introducing some hidden variable which was not obvisous/explicit.

Yes, this is exactly the issue, the hidden nature of it made it difficult to know what the state of the data is at any given moment. Especially since the pointers were set on the C++ side, then it's impossible know from the Python side what pointers exist and don't exist.

Actually it's kind of this way with the force field / potential grids as well. Maybe these could be made into something that can be managed on the Python side as well.

Usually I try to avoid changes to function interfaces especially on the python/C++ interface, and keep the argument list short.

If we have some common parameter lists that we pass to the functions, then maybe these could be packed into structs as well to keep the function signatures shorter. The structs can be easily extended as well without changing the function signature.

NikoOinonen · 2024-10-08T06:30:50Z

@ProkopHapala, you already approved this, but since I made some changes, I'll wait a bit if you want to comment on something before I merge.

ProkopHapala

OK

ProkopHapala · 2024-10-08T06:54:43Z

ppafm/core.py

+        self.rff_xs = np.ctypeslib.as_ctypes(self.np_rff_xs)
+        self.rff_ydys = np.ctypeslib.as_ctypes(self.np_rff_ydys.reshape(-1))
+
+    @classmethod


I did not know this pattern @classmethod before. Basically it is like overloaded constructor (?)

Kind of, yes. It makes the method so that it can be called without creating an instance of the class, and the first argument, instead of being self pointing to the instance, is cls which points to the class constructor.

ProkopHapala · 2024-10-08T06:57:21Z

u already approved this, but since I made some changes, I'll wait a bit if you want to comment on something before I merge.

I think it is good, go ahead.

(In general, I don't want to block you, inc case I forget to look on it.)

ProkopHapala · 2024-10-08T07:07:49Z

Especially since the pointers were set on the C++ side, then it's impossible know from the Python side what pointers exist and don't exist.

Yes, I think if we use something else than ctypes for interfacing (e.g. pybind11 ? ) this can be better. But I wanted something which does not have dependences, and it is general (even if we connect the C++ core to e.g. Julia, or use it as general dynamic c-library). Again historically, I considered python just as some script to make working with the C++ core more convenient)

Actually it's kind of this way with the force field / potential grids as well. Maybe these could be made into something that can be managed on the Python side as well.

Talking about big structural changes, it would be nice to make everything into classes inside C/C++ so that we can dynamically create and destroy instances of ppafm simulator and keep multiple different instances in memory simultanously. I think for some applications it could be useful.

I that case I would make python classes which mimic the C++ classes and keep the same data fields, and destroy/nullify them in sync.

If we have some common parameter lists that we pass to the functions, then maybe these could be packed into structs as well to keep the function signatures shorter. The structs can be easily extended as well without changing the function signature.

perhaps yes. At the beggining I did not want to use structs because it looks a bit more complicated and add some boilerplate. But form some complexity it is probably worth it.

NikoOinonen · 2024-10-08T07:30:39Z

I that case I would make python classes which mimic the C++ classes and keep the same data fields, and destroy/nullify them in sync.

I think if we use something else than ctypes for interfacing (e.g. pybind11 ? )

Yes, if we want to do this, then ctypes does not work, since it only interfaces with pure C, not C++ features like classes. pybind11 is advertising interfacing with C++, so that would probably be the way to go.

NikoOinonen added 3 commits October 4, 2024 13:10

Move code in "human_eye" tests into functions.

7b0ab92

This seems to fix the segfault when running tests with pytest.

Move spline parameters into a struct and introduce a corresponding Py…

788ea05

…thon interface.

Enable all tests.

fc62270

NikoOinonen requested a review from ProkopHapala October 7, 2024 14:53

ProkopHapala approved these changes Oct 7, 2024

View reviewed changes

Stored the original numpy arrays as well in SplineParameters, and cha…

21acacd

…nged variable name.

ProkopHapala approved these changes Oct 8, 2024

View reviewed changes

NikoOinonen merged commit 5d47cb9 into main Oct 8, 2024
15 checks passed

NikoOinonen deleted the ci-segfault-fix branch October 8, 2024 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix spline interpolation segfaults. #309

Fix spline interpolation segfaults. #309

NikoOinonen commented Oct 7, 2024 •

edited

Loading

NikoOinonen commented Oct 7, 2024

ProkopHapala left a comment

ProkopHapala Oct 7, 2024

NikoOinonen Oct 8, 2024

ProkopHapala Oct 7, 2024

NikoOinonen Oct 8, 2024

ProkopHapala Oct 7, 2024

NikoOinonen Oct 8, 2024

ProkopHapala Oct 7, 2024 •

edited

Loading

NikoOinonen Oct 8, 2024

ProkopHapala Oct 7, 2024

NikoOinonen Oct 8, 2024

ProkopHapala commented Oct 7, 2024

NikoOinonen commented Oct 8, 2024

NikoOinonen commented Oct 8, 2024

ProkopHapala left a comment

ProkopHapala Oct 8, 2024

NikoOinonen Oct 8, 2024

ProkopHapala commented Oct 8, 2024 •

edited

Loading

ProkopHapala commented Oct 8, 2024 •

edited

Loading

NikoOinonen commented Oct 8, 2024


		parameters = common.PpafmParameters()

		print("TipRSpline.ini overrides harmonic tip")

Fix spline interpolation segfaults. #309

Fix spline interpolation segfaults. #309

Conversation

NikoOinonen commented Oct 7, 2024 • edited Loading

NikoOinonen commented Oct 7, 2024

ProkopHapala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProkopHapala Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProkopHapala commented Oct 7, 2024

NikoOinonen commented Oct 8, 2024

NikoOinonen commented Oct 8, 2024

ProkopHapala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProkopHapala commented Oct 8, 2024 • edited Loading

ProkopHapala commented Oct 8, 2024 • edited Loading

NikoOinonen commented Oct 8, 2024

NikoOinonen commented Oct 7, 2024 •

edited

Loading

ProkopHapala Oct 7, 2024 •

edited

Loading

ProkopHapala commented Oct 8, 2024 •

edited

Loading

ProkopHapala commented Oct 8, 2024 •

edited

Loading