Skip to content

Conversation

@freddieknets
Copy link
Collaborator

Description

Fixed a small bug in cdefs before compiling a CPU kernel.
The code would check if a cdef for this kernel already exists, however, it would do so by checking by the Python name instead of the name in C.

Furthermore, I added a space before and a ( after the cname to be searched for, as otherwise if kernels exist which have the name of the new kernel as a part, the new one would not be generated.

E.g. if there already is a kernel definition void LocalSegment_crossing_drift(double s); then the new kernel void crossing_drift(double s1, double s2); would never be generated. Searching for f" {cname}(" avoids this problem.

All xobjects tests pass, and all xtrack tests with ContextCpu as well (did not test the other contexts as they are not affected).

Checklist

Mandatory:

  • I have added tests to cover my changes
  • All the tests are passing, including my new ones
  • I described my changes in this PR description

Optional:

  • The code I wrote follows good style practices (see PEP 8 and PEP 20).
  • I have updated the docs in relation to my changes, if applicable
  • I have tested also GPU contexts

Copy link
Contributor

@szymonlopaciuk szymonlopaciuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Definitely checking for pyname seems like a wrong thing to do.

I was wondering if we could add this check earlier when we construct cdefs but it seems it built partially with arbitrary string input, so it doesn't seem feasible.

@szymonlopaciuk szymonlopaciuk mentioned this pull request Dec 17, 2024
@szymonlopaciuk szymonlopaciuk merged commit 11d6bd4 into xsuite:main Dec 17, 2024
0 of 2 checks passed
@freddieknets freddieknets deleted the FixCDefs branch October 13, 2025 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants