First step in an attempted cleanup of the CLI part - some changes in the C code and Python/C interface#376
Conversation
Changes to be committed: modified: ppafm/cpp/Grid.h modified: ppafm/cpp/GridUtils.cpp modified: ppafm/cpp/ProbeParticle.cpp
…at used an arbitrary offset with an explicit special treatment of negative r.x, r.y, r.z Reformat the spacing in some functions from Grid.h
Change "nxy" to "ntot" in ppafm/cpp/Grid.h Fix the typo "N_arry" -> "N_array" in ppafm/GridUtils.py
NikoOinonen
left a comment
NikoOinonen
left a comment
There was a problem hiding this comment.
Thanks @mondracek, I had a few comments here.
ppafm/cpp/Grid.h
Outdated
| int nx = n.x; int ny = n.y; int nz = n.z; | ||
| int imx, imy, imz, itx, ity, itz; | ||
| double mx, my, mz, tx, ty, tz; | ||
| if(r.x >= 0) {imx = r.x; tx = r.x - imx; mx = 1 - tx; imx %= nx;} | ||
| else {itx = r.x; mx = itx - r.x; tx = 1 - mx; imx = itx % nx + nx - 1;} | ||
| itx = (imx + 1) % nx; | ||
| if(r.y >= 0) {imy = r.y; ty = r.y - imy; my = 1 - ty; imy %= ny;} | ||
| else {ity = r.y; my = ity - r.y; ty = 1 - my; imy = ity % ny + ny - 1;} | ||
| ity = (imy + 1) % ny; | ||
| if(r.z >= 0) {imz = r.z; tz = r.z - imz; mz = 1 - tz; imz %= nz;} | ||
| else {itz = r.z; mz = itz - r.z; tz = 1 - mz; imz = itz % nz + nz - 1;} | ||
| itz = (imz + 1) % nz; |
There was a problem hiding this comment.
It looks like this code is identical to the thing above. Maybe this could be an inline function to avoid repeating the logic, and no chance of forgetting to change one when making changes to the other.
There was a problem hiding this comment.
Okay, good point. The former version of the code had this repetition too but it was somewhat shorter so it did not look so much as a problem. I will think about the way how to solve this. Hold the merge until I address this issue.
There was a problem hiding this comment.
I have reduced the code repetition inside the functions interpolate3DWrap and interpolate3DVecWrap by introducing a C macro with argument, SET_GRIN_IND(a), in ppafm/cpp/Grid.h. Please check if you are okay with the solution.
There was a problem hiding this comment.
Note: I have chosen a macro instead of an inline function because an inline functions would require many paramaters, some of them passed by reference, like inline void set_grid_ind(double r, int n, int *im, int *it, double *m, double *t)
| elif len(FF.shape) == 3 or FF.shape[-1] == 1: | ||
| if verbose: | ||
| print("setFF() Creating a pointer to a scalar field") | ||
| core.setFF_Epointer(FF) |
There was a problem hiding this comment.
It looks like this can clash with the core.setFF_Epointer(V) below when computeVpot == True.
There was a problem hiding this comment.
That is true. I can put a safeguard there by forcing computeVpot = False whenever this branch is executed.
There was a problem hiding this comment.
Maybe also add some warning message, so that it does not simply happen silently.
There was a problem hiding this comment.
I have added both the computeVpot = False assignment and a warning to the elif branch for scalar FF in setFF():
elif len(FF.shape) == 3 or FF.shape[-1] == 1:
if verbose:
print("setFF() Creating a pointer to a scalar field")
core.setFF_Epointer(FF)
if computeVpot:
print("WARNING in setFF: computeVpot required but ignored because FF itself is scalar!")
computeVpot = False
I have implemented the warning with a simple print() command. I expect that we will change it to a more proper way after merging with your (@NikoOinonen) PR #368.
There was a problem hiding this comment.
I expect that we will change it to a more proper way after merging with your (@NikoOinonen) PR #368.
Yes, I can take care of that.
…polate3DWrap and interpolate3DVecWrap
NikoOinonen
left a comment
NikoOinonen
left a comment
There was a problem hiding this comment.
Looks good to me now
3 participants
xoff,yoff, andzoffoffsets in theinterpolate3DWrapfunction (inppafm/cpp/Grid.h)HighLevel.prepareArrays(),core.setFF_shape()andcore.setFF()into a new functionHighLevel.setFF()This is the first of a projected series of Pull Requests, intended to gradually replace the failed PR #347 and then go beyond in settling issue #198.
I have tested
examples/Grapheneandexamples/pyridineDensOverlap, both with CLI and OCL.