Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change issavedet Flag to Be Parsed as an Int in PMCX #237

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

matinraayai
Copy link
Contributor

  • Parse issavedet field of the cfg dict as an int instead of a bool.

Check List

Before you submit your pull-request, please verify and check all below items

  • You have only modified the minimum number of lines that are necessary for the update; you should never add/remove white spaces to other lines that are irrelevant to the desired change.
  • You have run make pretty (requires astyle in the command line) under the src/ folder and formatted your C/C++/CUDA source codes before every commit; similarly, you should run python3 -m black *.py (pip install black first) to reformat all modified Python codes, or run mh_style --fix . (pip install miss-hit first) at the top-folder to format all MATLAB scripts.
  • Add sufficient in-code comments following the doxygen C format
  • In addition to source code changes, you should also update the documentation (README.md, mcx_utils.c and/or mcxlab.m) if any command line flag was added or changed.

If your commits included in this PR contain changes that did not follow the above guidelines, you are strongly recommended to create a clean patch using git rebase and git cherry-pick to prevent in-compliant history from appearing in the upstream code.

Moreover, you are highly recommended to

  • Add a test in the mcx/test/testmcx.sh script, following existing examples, to test the newly added feature; or add a MATLAB script under mcxlab/examples to gives examples of the desired outputs
  • MCX's simulation speed is currently limited by the number of GPU registers. In your change, please consider minimizing the use of registers by reusing existing ones. CUDA compilers may not be able to optimize register counts, thus require manual optimization.
  • Please create a github Issue first with detailed descriptions of the problem, testing script and proposed fixes, and link the issue with this pull-request

Please copy/paste the corresponding Issue's URL after the below dash

This PR aims to fix an issue with how the issavedet is parsed in pmcx.cpp. Currently issavedet is parsed as a bool, wheras it should be parsed as an int_. This causes issues with specifying early-stopping criteria for pmcx simulations.

Credit to Chiara Motto to bringing this issue to my attention.

MWE to verify this change:

import numpy as np
import matplotlib.pyplot as plt
import pmcx
 
cfg = {
        'nphoton': 1e8,
        'vol': np.ones([60, 60, 60], dtype='uint8'),
        'tstart': 0,
        'tend': 9.77e-12*2000,
        'tstep': 9.77e-12,
        'unitinmm': 1,
        'srcpos': [30,30,0],
        'srcdir': [0,0,1],                            
        'prop': [[0, 0, 1, 1], [0.01, 10, 0.9, 1.55]],                            
        'seed': 0,
        'issrcfrom0': 1,
        'detpos':  [30, 20, 0, 1],           
        'maxdetphoton': 1e3,
        'savedetflag': 'p',
        'respin': 1,
        'issavedet': 3
        }
 
res = pmcx.mcxlab(cfg)

Output with the current pmcx:

nphoton: 1e+08
tstart: 0
tstep: 9.77e-12
tend: 1.954e-08
maxdetphoton: 1000
respin: 1
issrcfrom0: 1
unitinmm: 1
issavedet: 1
 
###############################################################################
#                      Monte Carlo eXtreme (MCX) -- CUDA                      #
#          Copyright (c) 2009-2024 Qianqian Fang <q.fang at neu.edu>          #
#                https://mcx.space/  &  https://neurojson.io/                 #
#                                                                             #
# Computational Optics & Translational Imaging (COTI) Lab- http://fanglab.org #
#   Department of Bioengineering, Northeastern University, Boston, MA, USA    #
###############################################################################
#    The MCX Project is funded by the NIH/NIGMS under grant R01-GM114365      #
###############################################################################
#  Open-source codes and reusable scientific data are essential for research, #
# MCX proudly developed human-readable JSON-based data formats for easy reuse.#
#                                                                             #
#Please visit our free scientific data sharing portal at https://neurojson.io/#
# and consider sharing your public datasets in standardized JSON/JData format #
###############################################################################
$Rev::eae5b2$v2024.2 $Date::2024-03-15 00:07:15 -04$ by $Author::Qianqian Fang$
###############################################################################
- code name: [Fermi MCX] compiled by nvcc [10.2] for CUDA-arch [350] on [Mar 15 2024]
- compiled with: RNG [xorshift128+] with Seed Length [4]
 
GPU=1 (NVIDIA GeForce RTX 4080 SUPER) threadph=305 extra=57600 np=100000000 nthread=327680 maxgate=2000 repetition=1
initializing streams ...   init complete : 1 ms
requesting 1536 bytes of shared memory
launching MCX simulation for time window [0.00e+00ns 1.95e+01ns] ...
simulation run# 1 ...
kernel complete:            6479 ms
retrieving fields ...           WARNING: the detected photon (126857) is more than what your have specified (1000), please use the -H option to specify a greater number  transfer complete:        6844 ms
normalizing raw data ...               source 1, normalization factor alpha=1023.541443
data normalization complete : 8357 ms
simulated 100000000 photons (100000000) with 327680 threads (repeat x1)
MCX simulation speed: 15617.68 photon/ms
total simulated energy: 100000000.00 absorbed: 42.85770%
(loss due to initial specular reflection is excluded in the total)

Expected Output (this patch):

nphoton: 1e+08
tstart: 0
tstep: 9.77e-12
tend: 1.954e-08
maxdetphoton: 1000
respin: 1
issrcfrom0: 1
unitinmm: 1
issavedet: 3
###############################################################################
#                      Monte Carlo eXtreme (MCX) -- CUDA                      #
#          Copyright (c) 2009-2024 Qianqian Fang <q.fang at neu.edu>          #
#                https://mcx.space/  &  https://neurojson.io/                 #
#                                                                             #
# Computational Optics & Translational Imaging (COTI) Lab- http://fanglab.org #
#   Department of Bioengineering, Northeastern University, Boston, MA, USA    #
###############################################################################
#    The MCX Project is funded by the NIH/NIGMS under grant R01-GM114365      #
###############################################################################
#  Open-source codes and reusable scientific data are essential for research, #
# MCX proudly developed human-readable JSON-based data formats for easy reuse.#
#                                                                             #
#Please visit our free scientific data sharing portal at https://neurojson.io/#
# and consider sharing your public datasets in standardized JSON/JData format #
###############################################################################
$Rev::      $v2024.6 $Date::                       $ by $Author::             $
###############################################################################
- code name: [Fermi MCX] compiled by nvcc [12.6] for CUDA-arch [520] on [Jan 10 2025]
- compiled with: RNG [xorshift128+] with Seed Length [4]

GPU=1 (NVIDIA GeForce RTX 2060) threadph=1627 extra=37120 np=100000000 nthread=61440 maxgate=2000 repetition=1
initializing streams ...        init complete : 13457 ms
requesting 1536 bytes of shared memory
launching MCX simulation for time window [0.00e+00ns 1.95e+01ns] ...
simulation run# 1 ... 
kernel complete:        13857 ms
retrieving fields ...   detected 1000 photons, total: 1000      transfer complete:      18180 ms
normalizing raw data ...        source 1, normalization factor alpha=131403.859375
data normalization complete : 27700 ms
simulated 100000000 photons (100000000) with 61440 threads (repeat x1)
MCX simulation speed: 2082.70 photon/ms
total simulated energy: 778928.00       absorbed: 43.09591%
(loss due to initial specular reflection is excluded in the total)

@fangq please verify this on your end and merge. Thanks!

- Parse issavedet field of the cfg dict as an int instead of a bool.
@kalvdans
Copy link

Update the documentation in its comment to describe what it means for issavedet to be 0, 1, 2 and 3. It should probably be renamed to start with something else than "is".

@matinraayai matinraayai changed the title pmcx.cpp: Change issavedet Flag to Be Parsed as an Int in PMCX Jan 10, 2025
@matinraayai
Copy link
Contributor Author

Update the documentation in its comment to describe what it means for issavedet to be 0, 1, 2 and 3. It should probably be renamed to start with something else than "is".

This is already documented in the mcxlab docs:

mcx/mcxlab/README.txt

Lines 315 to 318 in 8f433e9

cfg.issavedet: if the 2nd output is requested, this will be set to 1; in such case, user can force
setting it to 3 to enable early termination of simulation if the detected photon
buffer (length controlled by cfg.maxdetphoton) is filled; if the 2nd output is not
present, this will be set to 0 regardless user input.

I'm not sure if pmcx has a dedicated docs. I'm also not going to change the field's name since this PR is just fixing a parsing issue, not renaming the cfg field.

@kalvdans
Copy link

Update the documentation in its comment to describe what it means for issavedet to be 0, 1, 2 and 3. It should probably be renamed to start with something else than "is".

This is already documented in the mcxlab docs:

mcx/mcxlab/README.txt

Lines 315 to 318 in 8f433e9

cfg.issavedet: if the 2nd output is requested, this will be set to 1; in such case, user can force
setting it to 3 to enable early termination of simulation if the detected photon
buffer (length controlled by cfg.maxdetphoton) is filled; if the 2nd output is not
present, this will be set to 0 regardless user input.

Aha! I was looking at

char issavedet; /**<1 to count all photons hits the detectors*/

Case closed :)

@fangq fangq merged commit 4f20523 into fangq:master Jan 11, 2025
27 checks passed
@fangq
Copy link
Owner

fangq commented Jan 11, 2025

thank you @matinraayai, patch merged.

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