Proposed fix to improve pybind11 dynamic type casting #85

The9Cat · 2024-09-26T22:28:57Z

Problem

The pyEXP.coefs.setMatrix() routines fail to cast ndarray with complex type to Eigen::MatrixXd. Not all compiles demonstrated the problem. But recent compiles in the Docker container are affected.

Proposed fix

Use Eigen::Ref<Eigen::ComplexXcd> to pass arrays in setMatrix() rather than const Eigen::MatrixXcd&. Tests show that this fixed the issue.

To Do
The implementation has been verified on the failing case but more testing is needed.

…n pyEXP

… to set tensors

The9Cat · 2024-09-28T16:32:51Z

Changed the binding interface to allow both copies and references. This makes implies extra copies of the setMatrix() data blocks, but probably a worthwhile trade off for ease of use.

The9Cat · 2024-09-28T16:53:46Z

To Do

Confirm that these changes compiling correctly and solve the setMatrix() issues both natively and in the Docker container.
Consider adding a regression test to CMake tests

michael-petersen

I've been working with this branch for two weeks, and no workflows have been affected. I wasn't demonstrating the problem originally, so as long as that is reported fixed, I'm happy.

michael-petersen · 2024-10-10T18:53:39Z

pyEXP/FieldWrappers.cc

@@ -196,7 +196,8 @@ void FieldGeneratorClasses(py::module &m) {

        See also
        --------
-        slices : generate fields in a surface slice given by the initializtion grid
+        slices : generate fields in a surface slice given by the
+                 initializtion grid


Suggested change

initializtion grid

initialization grid

Good catch, thanks!

michael-petersen · 2024-10-10T18:53:50Z

pyEXP/FieldWrappers.cc

@@ -281,7 +282,8 @@ void FieldGeneratorClasses(py::module &m) {

        See also
        --------
-        slices : generate fields in a surface slice given by the initializtion grid
+        slices : generate fields in a surface slice given by the
+                 initializtion grid


Suggested change

initializtion grid

initialization grid

Good catch, thanks!

michael-petersen · 2024-10-10T18:54:02Z

pyEXP/FieldWrappers.cc

@@ -314,7 +316,8 @@ void FieldGeneratorClasses(py::module &m) {

        See also
        --------
-        slices : generate fields in a surface slice given by the initializtion grid
+        slices : generate fields in a surface slice given by the
+                 initializtion grid


Suggested change

initializtion grid

initialization grid

I have now added CTest pyEXP exercises for setMatrix() and createFromArray(). It seems that this bug bit Sóley while running a test notebook from Nico. Ooops.

The9Cat · 2024-10-12T15:41:52Z

Summary for this final (hopefully) commit

Fix a typo in the test dependencies for expNbodyTest (erroneously called makeNbodyTest in two locations)
Add a Python script to read a coefficient file, use setMatrix() to zero all odd-order coefficients and confirm that the new coefficieint object has zero odd-order coefficients.
Add a Python script to read a body file and use createFromArray() to make a coefficient structure and add it to a coefficient object.
Checked that all tests succeed

Probably good to go now...

michael-petersen · 2024-10-13T07:36:23Z

Great addition of tests!

The tests pass locally for me, but why are they failing in CI? That nothing is getting printed in Halo/createCoefs.py suggests to me the failure is early on.

michael-petersen · 2024-10-13T07:46:01Z

tests/Halo/createCoefs.py

@@ -44,11 +44,11 @@
    ypos.append(random.random()*2.0 - 1.0)
    zpos.append(random.random()*2.0 - 1.0)

-exit(0)
-
 print("---- createFromArray usings lists")
 coef1 = basis.createFromArray(mass, [xpos, ypos, zpos], time=3.0)


I guess the failure in the CI tests must be coming from this line -- but I can't reproduce it.

That was inserted to try to bisect the error. Thought I removed that.

Not sure what to do here; if we can't reproduce the error except in the runner where we can't debug it. Not really a solution, but the test seems to work fine on native Ubuntu builds, in the Docker container, and for you with Clang, right?

I'm going to label this test "long" and merge to main unless you have another idea?

Okay, I've figured it out, and fixed the script. In short, the Python numpy array objects need to persist. Even though I got rid of the Eigen::Ref which was hosing the interface in some builds, the data is passed by reference but if the data is wiped by an assignment, the memory gets stomped on access from the C++ side. To test, I changed the script to 'hold' the instance by giving it a unique name, and the script now works in CI.

However, this could lead to Python users writing code that breaks, depending on their workflow. So now, the question is: should we eliminate pass by reference for setMatrix() and createFromAarray() and force copies? I'm sort of thinking, yes?

I think the use case of a non-unique name is probably going to happen, so it makes sense to eliminate the pass by reference. The copies can't be that large (at least compared to RAM, I hope), so probably no harm in the copying strategy.

One more try just to make sure that this fails on all `createFromArrry` statements.

More bisection...

…emove debugging statements from createCoefs.py

to reassign `pyEXPCoefCreateTest` to `long`

The9Cat · 2024-10-14T23:54:58Z

Summary

Linked the bug in the github CI runner to a pybind11 issue

Details

I built the act local github runner generator with a tweaked build.yml that built with with debugging symbols and included gdb. When the code died in the same way (with a seg. fault), I logged into the docker image and fired up gdb. I was able to trace the failure to a pybind11 wrapper for an eigen3 object that was pointing at a bad address. I then updated the pybind11 git submodule and that fixed the problem! Still not sure why it failed in this particular build and not others, but I feel more comfortable that the native local builds are consistent with the github CI at this point.

To Do

I suppose we should make sure that nothing else is broken but that does seem unlikely at this point. Then I'm hoping we can merge.

Reverted from `Debug` to `Release`

The9Cat · 2024-10-15T15:07:02Z

Another (very minor) question is: do we leave the build.yml as it is current (with the 4 builds) or scale it back? Currently, I believe that all four are identical: ubuntu-latest is ubuntu-22.04 and openmpi will use gcc for mpicc anyway, so I don't think those are probing anything new. At some point, ubuntu-latest will be ubuntu-24.04, I assume. Then that might be useful since ubuntu-22.04 images are still used in the wild.

michael-petersen · 2024-10-15T15:24:05Z

I think we want some sort of matrix; currently ubuntu-latest is 24.04 (at least according to this log), but I agree with you on mpicc. One option would be to replace that with clang, but I imagine that would fail unless the rest of the environment was built against clang?

The9Cat · 2024-10-15T17:51:33Z

The log file from the ubuntu-latest run shows 22.04. But, I assume that it will be 24.04 at some point. I'll try the clang tag in act and see what happens...I think recent clang versions should work on straight Ubuntu.

test gcc and clang matrix

Another try.

Need OpenMP support for clang

Need flags for clang++ to use stdc++ lib.

Remove hardwired use of llvm's libc++

Use apt numpy package for 24.04

The9Cat · 2024-10-15T22:11:57Z

Summary of last bunch of commits

Updated build.yml to run gcc and clang C and C++ compiler pairs using include and exclude stanzas
Updated the top-level CMakeLists.txt to eliminate the previous default setting of the clang++ -stdlib=... flag. This should be set, if needed, by the local build configuration.
Fixed the missing C++ override decoration to eliminate chatty warnings from clang++. Some additional non-standard (according to clang++) grammar can be fixed later. The standard says that sizes must be known at compile time but most compilers allow this. At this point, there is no clean workaround. We could use combinations of std::vector<> and auto array = std::make_unique<int[]>(size); for this where size is a variable if we really want to comply. I have a commit for this stuff, but I'm reluctant to push that here without explicit testing.

…dentified by Clang and Clang++. They are all minor. The result passes 'ctest -L long' but pyEXP-example notebooks have not been run.

…ompliance

The9Cat · 2024-10-16T16:50:59Z

Looks like we are good to go. All merged and checked.

Martin D. Weinberg added 4 commits August 7, 2024 17:10

Implementation of a point set evaluator for FieldGenerator

76cdbd5

Use Eigen::Ref<> to pass arrays to prevent pybind11 mapping failure i…

9739781

…n pyEXP

Change names of Eigen::Tensor-->ndarray generators for clarity

12176dc

Updates for setMatrix() method for field coefficient types which need…

01516c3

… to set tensors

michael-petersen approved these changes Oct 10, 2024

View reviewed changes

Added CTests for 'setMatrix()' and 'createFromArray' [no ci]

14a6ef5

Martin D. Weinberg added 16 commits October 12, 2024 11:42

Typo in test label

1a65ec6

Use random data rather than depend on a body file

eb4bfd3

Get some verbose output from github runner

b81da9a

Make ctest log the output on failure

88d1338

Remove intentional failure from test script

41db618

Add another print statement

b937795

Add another print statement

dd78429

Exit after basis creation; sanity check

e9600e4

Exit after data creation; sanity check

eaa6521

Exit after first coef creation; sanity check

6432520

Exit after third coef creation; sanity check

8d332a2

Exit after second coef creation; sanity check

2369691

Comment out second coef creation; sanity check

747e6a5

Comment out all but first coef creation; sanity check

1043436

Restore original test script

278e1fe

Merge branch 'pybind11fix' of github.com:EXP-code/EXP into pybind11fix

856a547

michael-petersen reviewed Oct 13, 2024

View reviewed changes

Martin D. Weinberg and others added 3 commits October 13, 2024 10:34

Label creatCoefs test as 'long' to prevent CI failure

21045a6

Update createCoefs.py

4c27205

One more try just to make sure that this fails on all `createFromArrry` statements.

Update createCoefs.py

6853072

More bisection...

michael-petersen and others added 5 commits October 14, 2024 13:39

restore coef check to quick to induce failures

f9c0bac

Rollback to reference version of setMatrix() and createFromArray(); r…

bfbfbc8

…emove debugging statements from createCoefs.py

Update CMakeLists.txt

d2866db

to reassign `pyEXPCoefCreateTest` to `long`

git submodule updated

a007705

Relabel test as 'quick' after updating pybind11

02d09fb

Update build.yml

b215b49

Reverted from `Debug` to `Release`

The9Cat and others added 7 commits October 15, 2024 15:09

Update build.yml

ac91538

test gcc and clang matrix

Update build.yml

cdcab97

Another try.

Update build.yml

28fa507

Need OpenMP support for clang

Update build.yml

ae7fa0a

Need flags for clang++ to use stdc++ lib.

Update CMakeLists.txt

60ffa3d

Remove hardwired use of llvm's libc++

Decrease of the clang chatter; add 24.04 explicitly to the CI matrix

7d9cf63

Update build.yml

136fa16

Use apt numpy package for 24.04

Martin D. Weinberg added 2 commits October 15, 2024 20:34

This commit addresses all of the warnings and non-compliance issues i…

0218739

…dentified by Clang and Clang++. They are all minor. The result passes 'ctest -L long' but pyEXP-example notebooks have not been run.

Update for 24.04: remove pip install in favor of apt install of numpy

755bd90

The9Cat mentioned this pull request Oct 16, 2024

Clang compliance #90

Merged

michael-petersen and others added 6 commits October 16, 2024 10:38

custom MacOS fix for fftw_complex destructor problems

e712207

require FFTW

693c1f9

Use reinterpret cast from std::complex to handle FFTW complex types

455c03f

Merge branch 'ClangCompliance' of github.com:EXP-code/EXP into ClangC…

e735406

…ompliance

Merge of pointMesh; fix reflection in BiorthBasis

a806434

Merge branch 'pybind11fix' into ClangCompliance

9c6a72d

The9Cat merged commit 922d165 into main Oct 16, 2024
12 checks passed

The9Cat deleted the pybind11fix branch October 16, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposed fix to improve pybind11 dynamic type casting #85

Proposed fix to improve pybind11 dynamic type casting #85

The9Cat commented Sep 26, 2024

The9Cat commented Sep 28, 2024

The9Cat commented Sep 28, 2024 •

edited

Loading

michael-petersen left a comment

michael-petersen Oct 10, 2024

The9Cat Oct 12, 2024

michael-petersen Oct 10, 2024

The9Cat Oct 12, 2024

michael-petersen Oct 10, 2024

The9Cat Oct 12, 2024

The9Cat Oct 12, 2024

The9Cat commented Oct 12, 2024

michael-petersen commented Oct 13, 2024

michael-petersen Oct 13, 2024

The9Cat Oct 13, 2024

The9Cat Oct 13, 2024

michael-petersen Oct 13, 2024

The9Cat commented Oct 14, 2024 •

edited

Loading

The9Cat commented Oct 15, 2024

michael-petersen commented Oct 15, 2024

The9Cat commented Oct 15, 2024 •

edited

Loading

The9Cat commented Oct 15, 2024

The9Cat commented Oct 16, 2024

Proposed fix to improve pybind11 dynamic type casting #85

Proposed fix to improve pybind11 dynamic type casting #85

Conversation

The9Cat commented Sep 26, 2024

The9Cat commented Sep 28, 2024

The9Cat commented Sep 28, 2024 • edited Loading

michael-petersen 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

Choose a reason for hiding this comment

The9Cat commented Oct 12, 2024

michael-petersen commented Oct 13, 2024

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

The9Cat commented Oct 14, 2024 • edited Loading

The9Cat commented Oct 15, 2024

michael-petersen commented Oct 15, 2024

The9Cat commented Oct 15, 2024 • edited Loading

The9Cat commented Oct 15, 2024

The9Cat commented Oct 16, 2024

The9Cat commented Sep 28, 2024 •

edited

Loading

The9Cat commented Oct 14, 2024 •

edited

Loading

The9Cat commented Oct 15, 2024 •

edited

Loading