Initial support of the hyperboloid gaussian source in MCXLAB #127
Conversation
There was a problem hiding this comment.
@ShijieYan, can you add the acknowledgement to Gijs Buist, who submitted the initial patch, in both the source code comment as well as the commit's comment? you can simply add "patch submitted by Gijs Buist" and give the google group thread URL.
src/mcx_core.cu
Outdated
| @@ -1205,6 +1205,55 @@ __device__ inline int launchnewphoton(MCXpos *p,MCXdir *v,MCXtime *f,float3* rv, | |||
| canfocus=0; | |||
| break; | |||
| } | |||
| case(MCX_SRC_TRUE_GAUSSIAN): { // Gaussian with Box-Muller | |||
There was a problem hiding this comment.
this line and the closing { does not align with the previous case block
There was a problem hiding this comment.
Not sure why this block of code is not aligned with the rest of the code in the pull request. It looks fine on my end: https://github.com/ShijieYan/mcx/blob/3f904e79b5d23a9e1ddf0c7c5cbfe74b89845da8/src/mcx_core.cu#L1208-L1256
There was a problem hiding this comment.
if you select the whitespaces on the previous lines, you can see they are two tabs, and these are all spaces. I know this is not ideal, can you use the preceding whitespaces like other blocks for now?
src/mcx_const.h
Outdated
| @@ -86,6 +86,7 @@ | |||
| #define MCX_SRC_SLIT 13 /**< a collimated line source */ | |||
| #define MCX_SRC_PENCILARRAY 14 /**< a rectangular array of pencil beams */ | |||
| #define MCX_SRC_PATTERN3D 15 /**< a 3D pattern source, starting from srcpos, srcparam1.{x,y,z} define the x/y/z dimensions */ | |||
| #define MCX_SRC_TRUE_GAUSSIAN 16 /**< Gaussian-beam with spot focus, scrparam1.{x,y,z} define beam waist, distance from source to focus, rayleigh range */ | |||
There was a problem hiding this comment.
please use MCX_SRC_HYPERBOLOID_GAUSSIAN macro
src/mcx_core.cu
Outdated
| float l=rsqrtf(r*r+gcfg->srcparam1.z*gcfg->srcparam1.z); | ||
|
|
||
| /** if beam direction is along +z or -z direction */ | ||
| float3 pd=float3(r*(cphi-tt*sphi), r*(sphi+tt*cphi), 0.f); // position displacement from srcpos |
There was a problem hiding this comment.
also, any chance to reuse some of the registers? I counted 14 new registers are added, this can create a burden to the compiler to optimize out given we are not using source-specific template now.
There was a problem hiding this comment.
I did not see any variables that can be reused expect for float3 *rv. However, I am not sure if I can do that because rv is reserved to hold srcpos for focusable source types. In addition, the register # (nvcc -arch=sm_35) and speed changes before and after this patch can found at: https://docs.google.com/spreadsheets/d/165MJBm-gz9cWbPrjo-zg_YJxRBCUJjLFPQ66MVHwIy8/edit?usp=sharing
There was a problem hiding this comment.
wonderful, glad to see there is no major impact to existing features.
There was a problem hiding this comment.
I tried several implementations but in the end the register count did not vary among them. It seems that the compiler did a good job. Therefore I simply picked the one with the best readability.
|
@fangq It seems that the commit message (except for the latest commit) can not be amended once pushed to the remote repository. Perhaps I need to submit a new pull request. |
ShijieYan
force-pushed
the
hyperboloid
branch
from
3f904e7
to
1aa2922
Compare
|
@fangq I have updated the code and commit messages accordingly. Let me know if you have any additional comments. |
|
Original patch submitted by Gijs Buist: https://groups.google.com/g/mcx-users/c/wauKd1IbEJE/m/_7AQPgFYAAAJ. |
No description provided.