Closed editorialbot closed 1 year ago
editorialbot
commented
1 year ago Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.10 s (800.6 files/s, 165825.3 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
HTML 25 935 75 6201
SVG 1 0 0 2671
Python 18 428 652 1211
JavaScript 12 131 213 871
CSS 4 185 35 762
reStructuredText 12 397 484 444
Markdown 4 100 0 211
TeX 1 11 0 113
YAML 1 5 11 10
-------------------------------------------------------------------------------
SUM: 78 2192 1470 12494
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 802
editorialbot
commented
1 year ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1016/j.neuroimage.2014.06.064 is OK
- 10.1002/nbm.4414 is OK
- 10.21105/joss.02527 is OK
- 10.1371/journal.pone.0076626 is OK
- 10.3389/fninf.2020.00008 is OK
- 10.1007/978-3-642-15745-5_50 is OK
- 10.1002/nbm.3496 is OK
- 10.1002/mrm.27101 is OK
- 10.1038/nn1075 is OK
MISSING DOIs
- None
INVALID DOIs
- None:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
jacobblum
commented
1 year ago Thank you all for being willing to review. If you have any questions, please let us know; we'll be happy to answer!
jgostick
commented
1 year ago I just pinged both by email, so hopefully they'll be dropping in here soon to communicate their progress.
djps
commented
1 year ago There are contributing guidelines, but no mention of them nor reporting issue or seeking support in the README
Some of the references are not refered to in the text, which is a bit confusing - should these be listed if they are not mentioned?
There could be some mention of other comparable codes as part of a state of the field. For example the disimpy code is referenced by not mentioned. The review paper: "Design and Validation of Diffusion MRI Models of White Matter" provides a useful overview of the theoretical background as well as many codes, so may be a useful starting point to reference, although is from 2017.
Pedantically, the formatting of the citation to Rafael-Pinto et al is a link only by the year of publication, where as the rest are by (Author, Year). Also on line 55 it may be $\sqrt{6D_0 \mathrm{d}t}$. Some of the references should have some capitalisation in the titles, for example {M}onte {C}arlo in disimpy or llvm, jit etc.
On line 39 it should read $\mu$m, without a space in between.
djps
commented
1 year ago @jacobblum - I can't seem to install it when following the instructions:
In windows 11, using conda 23.5.2, (with python 3.11), cuda driver (528.90) and nvcc 11.3.58 but can't install the cuda-toolkit.
It finds lots of conflicts which it can't resolve. What version of conda was this tested on? I had tried different channels also.
I was able to install it using pip though. The tests passed after 28 minutes. I could replicate the quick start examples as well.
jacobblum
commented
1 year ago @djps
Thanks for trying it out. The main conda version the installation was tested on is 23.3.1 (on a windows 10 machine with nvcc 11.7.64). Trying things locally again, I don't see the same behavior, but this is very likely conda/cudatoolkit/nvcc version specific issue.
To try to narrow down what's happening could we run through a few details?
Thanks so much!
djps
commented
1 year ago the conda environment was created with python 3.8.17.
The error was for the conda install cudatoolkit installation:
(simDRIFT) GitHub>conda install cudatoolkit
Collecting package metadata (current_repodata.json): done
Solving environment: failed with initial frozen solve. Retrying with flexible solve.
Solving environment: failed with repodata from current_repodata.json, will retry with next repodata source.
Collecting package metadata (repodata.json): done
Solving environment: failed with initial frozen solve. Retrying with flexible solve.
Solving environment: -
Found conflicts! Looking for incompatible packages.
This can take several minutes. Press CTRL-C to abort.
Examining numpy: 8%|████████████▌ | 3/37 [00:20<00:02, 13.90it/s]
Examining libhwloc: 19%|████████████████████████████▊ | 7/37 [00:25<02:09, 4.33s/it]|
Examining tk: 27%|██████████████████████████████████████████▍ | 10/37 [00:31<01:01, 2.27s/it]
Examining liblapack: 38%|████████████████████████████████████████████████████████▊ | 14/37 [00:54<00:34, 1.52s/it]
Examining zipp: 62%|████████████████████████████████████████████████████████████████████████████████████████████████▎ | 23/37 [01:49<01:39, 7.13s/it]|
Examining pthreads-win32: 73%|█████████████████████████████████████████████████████████████████████████████████████████████████████████▊ | 27/37 [02:00<00:24, 2.47s/it]
Examining libiconv: 92%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████▊ | 34/37 [02:04<00:09, 3.22s/it]
Examining conflict for libzlib numba numpy libffi xz libhwloc python importlib_metadata tk cudatoolkit setuptools wheel tbb pip libxml2 python_abi libblas zipp vc openssl importlib-metadata pthreads-win32 llv
Examining conflict for importlib_metadata numba importlib-metadata: 14%|██████████████ | 5/37 [01:02<06:42, 12.57s/it]|
Examining conflict for setuptools xz numba numpy zipp importlib-metadata python wheel importlib_metadata llvmlite pip python_abi: 35%|██████████████▍ | 13/37 [01:38<02:21, 5.88s/it]
Examining conflict for setuptools numba numpy libhwloc zipp importlib-metadata python wheel importlib_metadata llvmlite pip libxml2 python_abi: 41%|██████████▉ | 15/37 [01:52<02:10, 5.95s/it]
Examining conflict for setuptools numba numpy zipp importlib-metadata python wheel importlib_metadata tk llvmlite pip python_abi: 62%|█████████████████████████▍ | 23/37 [03:06<01:34, 6.79s/it]
Examining conflict for setuptools numba numpy zipp importlib-metadata python wheel importlib_metadata llvmlite pip python_abi libsqlite: 62%|█████████████████████▏ | 23/37 [03:19<01:34, 6.79s/it]|
Examining conflict for numpy libblas libcblas liblapack: 76%|██████████████████████████████████████████████████████████████████████████████████████▎ | 28/37 [04:00<01:26, 9.63s/it]
Examining conflict for setuptools numpy zipp importlib-metadata wheel importlib_metadata llvmlite pip python_abi: 84%|███████████████████████████████████████████████▊ | 31/37 [04:42<01:07, 11.30s/it]
failed
UnsatisfiableError: The following specifications were found to be incompatible with each other:
Output in format: Requested package -> Available versions
Package six conflicts for:
wheel -> packaging[version='>=20.2'] -> six
zipp -> more-itertools -> six[version='>=1.0.0,<2.0.0']
numpy -> mkl-service[version='>=2.3.0,<3.0a0'] -> six
importlib_metadata -> pathlib2 -> six[version='>=1.13.0']
importlib-metadata -> pathlib2 -> six[version='>=1.13.0']
pip -> html5lib -> six[version='>=1.9']and on and on for many packages until at the end:
Package pthreads-win32 conflicts for:
pthreads-win32
tbb -> libhwloc[version='>=2.9.1,<2.9.2.0a0'] -> pthreads-win32
libhwloc -> pthreads-win32The following specifications were found to be incompatible with your system:
- feature:/win-64::__win==0=0
- feature:|@/win-64::__win==0=0
- setuptools -> wincertstore[version='>=0.2'] -> __win
Your installed version is: 0
Note that strict channel priority may have removed packages required for satisfiability.so I can't proceed further.
jacobblum
commented
1 year ago Thanks! I'll look into this.
kainenutt
commented
1 year ago @jacobblum - I can't seem to install it when following the instructions:
In windows 11, using conda 23.5.2, (with python 3.11), cuda driver (528.90) and nvcc 11.3.58 but can't install the cuda-toolkit.
It finds lots of conflicts which it can't resolve. What version of conda was this tested on? I had tried different channels also.
I was able to install it using pip though. The tests passed after 28 minutes. I could replicate the quick start examples as well.
Hi @djps,
We are looking more into what may have caused the errors you experienced when using the installation instructions we provided. At present, we have been unable to replicate the error -- we will get back to you with further clarifying questions should we need more information.
In the meantime, we will update the installation instructions to use pip instead of conda to install cudatoolkit since it worked for you (as well as our tests).
Thank you! -Kainen
jgostick
commented
1 year ago Hey All, just checking in to see if things are progressing.
@mfroeling, have you started your review yet? You can create your own 'checklist' of things to look for using the instructions at the top (I won't type them here in case I accidentally trigger the generation of a checklist!).
mfroeling
commented
1 year ago mfroeling
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mfroeling
commented
1 year ago @kainenutt @jacobblum
Hi all I finally got around to evaluating the tool. It all seems to run but with some minor issues.
For me to check all the checklist items i think some things have to be addressed as listed below. Feel free to reach out if it needs more clarifications.
My major items are the documentation and statement of need / state of the field in the paper.
installation
examples although I'm not an python expert and hardly use it in the end i was able to run the examples
calling the tool
information needed
paper
Documentation
conda env export | grep -v "^prefix: " > environment.yml such that not each toolbox has to be installed manually as it is now. jacobblum
commented
1 year ago @jacobblum - I can't seem to install it when following the instructions:
In windows 11, using conda 23.5.2, (with python 3.11), cuda driver (528.90) and nvcc 11.3.58 but can't install the cuda-toolkit.
It finds lots of conflicts which it can't resolve. What version of conda was this tested on? I had tried different channels also.
I was able to install it using pip though. The tests passed after 28 minutes. I could replicate the quick start examples as well.
@djps, The error message you attached below precipitated by the conda install command indicates some sort of incompatibility with the llvmlite package, which binds Python to LLVM. The llvmlite readthedocs indicates that there are many potential sources of incompatibility between a given build of llvmlite and the LLVM release to which it is statically linked, which may be dependent on a machines architecture, among many other reasons (https://llvmlite.pydata.org/en/latest/faqs.html).
Unfortunately if this is the issue, I don't think there is much we can do about it in terms of completely resolving these incompatibilities among the dependencies. Luckily, however, given pip's less strict dependency verification processes (relative to conda), it appears that whatever incompatibility conda is finding, it is subtle and ultimately inconsequential given that the test suite still runs correctly when installation is facilitated by pip.
We have thus amended our installation (https://simdrift.readthedocs.io/en/latest/installation/index.html) documentation to make note of the potential architecture dependence, and specifically have deferred to Numba's installation guide, which covers the use of both pip and conda for installation purposes. This should help to avoid any hiccups with this step of the installation.
jacobblum
commented
1 year ago There are contributing guidelines, but no mention of them nor reporting issue or seeking support in the README
@djps We have now added these elements to the README. Thanks!
jgostick
commented
1 year ago hi @mfroeling, have you had the chance to play around with the software and start evaluating the checklist?
jacobblum
commented
1 year ago @mfroeling
Thank you for the very useful feedback, and for your patience while we addressed your comments. Please see our responses below. You will find that we have made many of the changes and clarifications you have suggested.
@kainenutt @jacobblum
Hi all I finally got around to evaluating the tool. It all seems to run but with some minor issues.
For me to check all the checklist items i think some things have to be addressed as listed below. Feel free to reach out if it needs more clarifications.
My major items are the documentation and statement of need / state of the field in the paper.
installation
- [ ] when installing what aspects of the syntax for the GPU needs changing and how can i figure it out
Thank you for your question. While I’m not sure I’m understanding the question exactly, I want to clarify that the simulation will only run on the GPU. Without an available Cuda device, an error message would be thrown after the command line call preventing the simulation from running. See main/src/cli.py, lines 110-111 for details.
- [ ] When running the example the matplotlib installation was not working. i reinstalled the matplotlib using conda and my environment was inconsistent, it was able to upgrade but it had to downgrade some packages (openssl cryptography and python). i would check the exported environement given in requirements.
Thank you for pointing this out! We have now included a compatible matplotlib version in the setup.py requirements. It will automatically install while setting up simDRIFT now.
- [ ] running the tests what is the expectation how long it should take or what its doing. the test ran for almost an hour on my laptop and what i could see only using the CPU. I have a CUDA enabled GPU put cannot test or see if it actually has been used. The test ran 20 items, i would like to see what is tested and maybe have an interface to test individual items.
Thank you for the comment here. We have included all test items in the documentation (https://simdrift.readthedocs.io/en/latest/test_suite/index.html) and added logging to the test suite so that it is clear what exactly is being tested. It is difficult to predict a runtime given the hardware dependence here; however, in the documentation we made note of the runtime on our hardware (about 8 minutes). We also have remarked that due to the small-scale of the test suite’s simulations, GPU utilization may not increase significantly even while the code is executing on the GPU. Like I mentioned in the above reply, the simulation cannot run without detecting and using the CUDA device.
- [ ] What system requirements are needed, can the tool be run on CPU only or does one need a GPU. how much memory does a typical simulation needs.
Thank you for the suggestion. The authors agree that this is great information to include, and so we have added this to the installation documentation (https://simdrift.readthedocs.io/en/latest/installation/index.html). Specifically, from a hardware standpoint we require a CUDA device with compute capability of 3.0 or higher. We have also made note that typical simulations do not exceed 2 Gb of VRAM use.
examples although I'm not an python expert and hardly use it in the end i was able to run the examples
- [ ] . However i was a bit confused that the tool has to be called from the conda command window while the evaluation of the output is done in python. Could you give some examples of how to run the tool directly from python instead of the cmd. I can include os and use the command prompt using os.system but there must be a more elegant way to do this.
Thank you for your suggestion. While the authors understand the point made here, we believe that it is easiest to use the tool from the command line. As you mention, calling the program from within a Python script is easily done using os.system. This is a common practice within our research group’s workflow in the context of simulation and using other tools like FSL from within a Python script.
We recommend using a conda environment to prevent conflict with any other Python packages you may have installed for use in other workflows. Conda is just a package manager here, all the work, as you mention, is done with Python.
- [ ] In manual it is not clear that where you run the tool this is also the folder where the output is placed. When running the examples the use of directories is a bit messy. And only based on the python code I could figure out that some things like the b-values are packed in the toolbox while the output is done in the folder where the code is evaluated. But nowhere in the help i found what outputs to expect when running the code. Apparently the signals are stored in the signals folder as *.nii and the trajectories in a different folder. But nowhere is specified how these outputs should look like or what is in them.
Thank you for the useful comment here. We have added a reference section to the documentation that specifically addresses program inputs and outputs (https://simdrift.readthedocs.io/en/latest/reference/index.html)
- [ ] Can i use custom paths to the bval and bvec files
Yes, instructions regarding formatting and inputting custom .bval and .bvec files are now covered in the reference section of the documentation: (https://simdrift.readthedocs.io/en/latest/reference/index.html)
- [ ] The logfiles shows what has been performed but not the command used to generate these results so if i want to reproduce them and i didn't save the command I gave in the cmd window i have no clue how to reproduce them.
This is a great point. We have included in the outputs a copy of the input arguments, as now noted in the documentation (https://simdrift.readthedocs.io/en/latest/reference/index.html)
calling the tool
- [ ] It is not clear to me what the default values are and which inputs are mandatory. I removed some of the inputs of the examples and it still runs.
We had previously implemented default inputs when the named parameter was omitted in the command line call. We have removed this and made note of all inputs in the reference section of the documentation (https://simdrift.readthedocs.io/en/latest/reference/index.html).
- [ ] Can the input settings move to a simulation config file where one can setup the simulation needed and just point to that file for all the inputs
This is a great suggestion, and we have implemented this functionality. As is covered in the reference section of the documentation, the simulation takes as input a INI configuration file that contains all important input parameters. Please see both the reference and quickstart section of the documentation for details.
- [ ] Is it possible to configure input and output directories for the simulation now things that were there are just overwritten so when running multiple simulations some management of input of output is needed or needs to be explained.
Thank you for the suggestion. This is a great idea, and the feature has now been implemented. Please see the reference or quickstart section of the documentation for details.
- [ ] from the manual it is not clear what the units of the inputs are. Diffusivity, bvalue, angles, time etc. etc. can all be specified in various methods, which units for each parameter does the tool assume
Thank you for the comment. All units have now been added to input parameter descriptions. Please see details in the reference section of the documentation (https://simdrift.readthedocs.io/en/latest/reference/index.html).
information needed
- [ ] which kind of diffusion weighting sequence is used (Stejskal Tanner?) does it take into account gradient slopes or does is assumes block gradients can the gradient wave forms be changed.
Thank you for the question. Throughout the documentation we now make note that we have implemented the PGSE (Stejskal Tanner) experiment with the narrow pulse approximation.
- [ ] the bvalues and bvecs used should use the .bval and .bvec extensions in line with most diffusion toolboxes. how do these files need to be formatted.
Thank you for the question. The authors have clarified .bval and .bvec file formatting in the reference section of the documentation.
- [ ] Is it possible to run the simulation with 3 fiber populations each along a different axes (i.e. x-axes, y-axes, z-axes) the examples given only show rotation of the fiber population over one axes. In the brain crossing in 3 directions happens in many locations and given the example 2 i don't see how this tool can simulate this with only one rotation angle theta.
While the authors understand the point made here, we wanted to avoid populating the simulated image voxel with overlapping fibers, which is achieved by allowing rotation along more than one axis. The major point of emphasis in our simulation was to test DWI inverse problem model’s ability to resolve crossing fiber angles. To an extent, we view all crossing angles as isomorphic to the geometry implemented in the simulation. Crossing angles along several axes is therefore beyond the scope of this simulation.
We also have not precluded the possibility of including a second rotation axis in future versions. However, the simulator can model a wide range of microstructural configurations as it stands, and thus we believe that it has merit even without the inclusion of a second axis of rotation.
paper
- [ ] according to JOSS Substantial scholarly effort the software should "be a significant contribution to the available open source software that either enables some new research challenges to be addressed or makes addressing research challenges significantly better". From the paper it is not fully clear to me what this tool can do different of other montecarlo simulation tools for diffusion such as CAMINO http://camino.cs.ucl.ac.uk/index.php?n=Tutorials.MCSimulator or difSim http://csci.ucsd.edu/software/difsim.html amongst others. The statement of need and state of the field are a bit non convincing for me. These kinds of simulations i have seen and used for years in various packages.
Thank you for the comment. A major point of emphasis in the simDRIFT statement of need is that this simulator, unlike others, is mesh free, allowing users to easily specify many potential image voxel configurations. By contrast, simulators like CAMINO rely on either already packaged or pre-supplied meshes for simulation, which can be difficult to make for complex voxel geometries. Therefore, simDRIFT allows users who lack experience or training in generating meshes for computational domains to simulate complex image voxel configurations. Another important difference is that this simulation runs on the GPU, while CAMINO runs single threaded on the CPU.
Documentation
- [ ] The documentation does not contain a statement of need.
We have included the statement of need in the documentation’s introduction section (https://simdrift.readthedocs.io/en/latest/introduction/index.html)
- [ ] Installation instructions are there, but they do not clearly list the needed dependencies in the toolbox. There is a conda/pip installation requirements.txt but i had to reinstall the matplotlib (my guess is due to the pytorch installation)
All dependencies should now be accounted for in the setup.py file.
- [ ] Ideally the authors make a clean environment install only the minimum and do
conda env export | grep -v "^prefix: " > environment.ymlsuch that not each toolbox has to be installed manually as it is now.
Thank you for your suggestion. The authors prefer the current installation instructions because they prompt the user to install appropriate NVIDIA drivers and hardware architecture specific versions of both Numba and Pytorch, without which the code would not run.
- [ ] The documentation lack clear interface description (input and output definitions), which variables are needed which units, which files have to be stored or found where. how can the output be configured. What does the tool output (signals, trajectories) what are those file definitions what is stored in them and how can the be used.
All this information should now be included in the reference section of the documentation, as noted above.
- [ ] Testing is present but obscure, not sure what is tested. 20 tests are run but it is not displayed what is tested and why.
Thank you for the comment here. We have included all test items in the documentation (https://simdrift.readthedocs.io/en/latest/test_suite/index.html) and added logging to the test suite so that it is clear what exactly is being tested, as described above.
mfroeling
commented
1 year ago @jacobblum thanks for the elaborate updat! ill have time later this week to review the updates and then I will get back to you.
mfroeling
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mfroeling
commented
1 year ago @jacobblum
The clarifications and updates realy helped. I reinstalled everything and was better able to understand the steps and examples since it is now showing clear feedback if the user wants.
I was also able to generate some extra simulations based on the examples, so im ticking all the boxes for the issues and review items.
However the paper one i am leaving open for now, since i believe this has not been improved or clarified. It is not that i dont see use for this tool but i would like to see it better explained what the extra / substantial scholarly effort is for this software.
below ill be a bit more specific and critical about the statements made .
Paper:
simDRIFT: Running simDRIFT
simDRIFT: ------------------------------
simDRIFT: Fiber Setup
simDRIFT: ------------------------------
simDRIFT: 144 fibers of type 0 (R0 = 1.0)
simDRIFT: 144 fibers of type 1 (R1 = 1.0)
simDRIFT: 361 fibers of type 2 (R2 = 1.0)
simDRIFT: Fiber geometry: Penetrating
simDRIFT: ------------------------------
simDRIFT: Rotation Matrices
simDRIFT: ------------------------------
simDRIFT: [[ 1.00, 0.00, 0.00],
simDRIFT: [ 0.00, 1.00, 0.00],
simDRIFT: [-0.00, 0.00, 1.00]]
simDRIFT: [[ 0.71, 0.00, 0.71],
simDRIFT: [ 0.00, 1.00, 0.00],
simDRIFT: [-0.71, 0.00, 0.71]]
simDRIFT: [[-0.71, 0.00, 0.71],
simDRIFT: [ 0.00, 1.00, 0.00],
simDRIFT: [-0.71, 0.00, -0.71]]
simDRIFT: ------------------------------
simDRIFT: Placing Cells...
simDRIFT: ------------------------------
simDRIFT: ------------------------------
simDRIFT: Cells Setup
simDRIFT: ------------------------------
simDRIFT: 11 cells with radius = 5.0 um
simDRIFT: 1 cells with radius = 10.0 um
simDRIFT: ------------------------------
simDRIFT: Empirical Volume Fractions
simDRIFT: ------------------------------
simDRIFT: Fiber 1 Volume: 0.060572
simDRIFT: Fiber 2 Volume: 0.052422
simDRIFT: Fiber 3 Volume: 0.125596
simDRIFT: Cell Volume: 0.040788
simDRIFT: ------------------------------
simDRIFT: Beginning Simulation...
simDRIFT: ------------------------------
simDRIFT: Simulation complete!
simDRIFT: Elapsed time: 268 seconds@editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
jgostick
commented
1 year ago Hi @jacobblum, I see you have some work to do to address @mfroeling's detailed comments. I'll keep an eye on this thread because it's getting close to finished.
jacobblum
commented
1 year ago @mfroeling Thank you for the useful feedback this morning. To provide clarity with respect to the substantial scholarly effort dimension of simDRIFT, we have significantly re-worked the Statement of Need section of the paper. In particular, we have added a paragraph comparing simDRIFT's performance with another mesh based, open source DWI forward simulation (Dismpy), previously published in JOSS (https://joss.theoj.org/papers/10.21105/joss.02527) and demonstrated to be substantially faster than CAMINO. Comparisons between simDRIFT and Disimpy on identical image voxels show orders of magnitude performance gains achieved by simDRIFT, especially for large numbers of spins (256,000, 512,000, 1 Million). Thus, the authors believe that simDRIFT may be especially useful for these types of large scale simulations, where other simulators (like Disimpy) are relatively slow.
The authors hope that you will agree that this result, along with other simDRIFT properties mentioned in the new statement of need section fulfill the substantial scholarly effort criteria.
@jacobblum
The clarifications and updates realy helped. I reinstalled everything and was better able to understand the steps and examples since it is now showing clear feedback if the user wants.
I was also able to generate some extra simulations based on the examples, so im ticking all the boxes for the issues and review items.
However the paper one i am leaving open for now, since i believe this has not been improved or clarified. It is not that i dont see use for this tool but i would like to see it better explained what the extra / substantial scholarly effort is for this software.
below ill be a bit more specific and critical about the statements made .
Paper:
- [ ] The only argument is that te community needs mesh free simulators. Which this tool indeed brings. But i dont think the meshing is the issue. The solution presented here is twofold i) remove mesh ii) parameterize the environement. The second argument allows for running many easy configurable experiemnts. However, one can also write some code that can easily generate meshes based on the same parameters.
- [ ] The second argument is the GPU implementation. Some simulaters run on CPU on multiple cores. If one does not have a GPU the tool cannot be used. So arguments can also be made for CPU implementation with paralelization. So i dont think dat choices of implementation can be an argument or unmet need. Computation speed could be but only if better supported.
Thank you for the useful comment here. Please refer to the updated statement of need section demonstrating simDRIFTs superior computational speed relative to an existing open source DWI forward simulator (Disimpy). For large scale simulations (say > 10^5 spins), simDRIFT achieves considerable performance advantages relative to Disimpy.
The authors strongly agree with the remark about CPU level-parallelization; however, with CPU parallelization the runtime is roughly a linear function of the number of spins. For spin ensembles exceeding even just 10^4 spins, to achieve reasonably fast simulations, many CPU cores would be needed. Anecdotally, on very early builds of simDRIFT that used CPU parallelization, runtimes on simulations performed on a 120 core CPU would typically be on the order of hours or even days depending on the micro-structural complexity of the voxel. With the GPU, simDRIFT is able to simulate more complex geometries much faster, and with standard desktop and laptop level hardware. We have added a comment specifically denoting this feature in the updated statement of need.
- [ ] With te new improvements i was able to simulate and play a bit. I realized no examples with cells were given so i tried those. However, I cannot specify the diffusivity within the cells. Also from the initial trajectories it seems the cells are overlapping with the fibers. I gave fiber fractions of .2,.2,.5 and cell fractios of .1, .1 which both are defined in the help as the "the volume of the imaging voxel occupied by..." which adds to more than 100%. Can different populations can occulpy the same volume?
The input volume fractions assume no overlapping between the micro-structural elements (e.g., fibers don't overlap with cells). When this is the case, as we would expect with non-trivial fiber and cell populations, the input volume fractions will not match the MC integration computed ones in the log file. To clarify, when overlapping occurs, the spins default to being inside of the fiber. We will add clarification in the documentation that input fiber / cell fractions are not necessarily exact.
- [ ] The paper also states "highly configurable" which is bit of a stretch since there is only one fiber rotation angle, no curving fibers within voxels (e.g. https://journals.plos.org/plosone/article?id=10.1371/journal.pone.0076626), etc. The actual Emperical volume fraction reported back to me dont match the ones i asked for and the cell fraction of both cells should be equal to the each of fiber 1 and 2. I gues this has to do with fiber and cell pakking, but if i want to simulate a realisticly observed vollume distribution this cannot be done. Yes in potential this framework can be expanded with all these cases but currently it is not.
You're exactly correct - for the fibers, we use square packing, which cannot achieve the same densities as better algorithms like hexagonal close packing (HCP). We plan to implement HCP in a future update to achieve better convergence of the simulated fiber fractions to the input values (for high densities especially). We similarly plan to improve the cell (sphere) packing routine.
Im not arguing the tool has no needs, indeed the forward simulation to validated inverse moddeling is a valid one. But the claims and statement of need should be better defined.
simDRIFT: Running simDRIFT simDRIFT: ------------------------------ simDRIFT: Fiber Setup simDRIFT: ------------------------------ simDRIFT: 144 fibers of type 0 (R0 = 1.0) simDRIFT: 144 fibers of type 1 (R1 = 1.0) simDRIFT: 361 fibers of type 2 (R2 = 1.0) simDRIFT: Fiber geometry: Penetrating simDRIFT: ------------------------------ simDRIFT: Rotation Matrices simDRIFT: ------------------------------ simDRIFT: [[ 1.00, 0.00, 0.00], simDRIFT: [ 0.00, 1.00, 0.00], simDRIFT: [-0.00, 0.00, 1.00]] simDRIFT: [[ 0.71, 0.00, 0.71], simDRIFT: [ 0.00, 1.00, 0.00], simDRIFT: [-0.71, 0.00, 0.71]] simDRIFT: [[-0.71, 0.00, 0.71], simDRIFT: [ 0.00, 1.00, 0.00], simDRIFT: [-0.71, 0.00, -0.71]] simDRIFT: ------------------------------ simDRIFT: Placing Cells... simDRIFT: ------------------------------ simDRIFT: ------------------------------ simDRIFT: Cells Setup simDRIFT: ------------------------------ simDRIFT: 11 cells with radius = 5.0 um simDRIFT: 1 cells with radius = 10.0 um simDRIFT: ------------------------------ simDRIFT: Empirical Volume Fractions simDRIFT: ------------------------------ simDRIFT: Fiber 1 Volume: 0.060572 simDRIFT: Fiber 2 Volume: 0.052422 simDRIFT: Fiber 3 Volume: 0.125596 simDRIFT: Cell Volume: 0.040788 simDRIFT: ------------------------------ simDRIFT: Beginning Simulation... simDRIFT: ------------------------------ simDRIFT: Simulation complete! simDRIFT: Elapsed time: 268 seconds
Again, thank you for all of the useful feedback. Please let us know what additional items, if any, you'd need to see improved / clarified. Thanks so much.
jacobblum
commented
1 year ago @djps In the most recent amendment to the simDRIFT manuscript, we have described and presented direct comparisons between simDRIFT and other existing open source software (Disimpy - https://joss.theoj.org/papers/10.21105/joss.02527). We hope that this satisfies this part of the checklist. Thanks so much, and please let us know what additional changes / clarifications, if any, need to be made.
Review checklist for @djps
- [ ] State of the field: Do the authors describe how this software compares to other commonly-used packages?
djps
commented
1 year ago @jacobblum - I can't seem to install it when following the instructions: In windows 11, using conda 23.5.2, (with python 3.11), cuda driver (528.90) and nvcc 11.3.58 but can't install the cuda-toolkit. It finds lots of conflicts which it can't resolve. What version of conda was this tested on? I had tried different channels also. I was able to install it using pip though. The tests passed after 28 minutes. I could replicate the quick start examples as well.
Hi @djps,
We are looking more into what may have caused the errors you experienced when using the installation instructions we provided. At present, we have been unable to replicate the error -- we will get back to you with further clarifying questions should we need more information.
In the meantime, we will update the installation instructions to use pip instead of conda to install cudatoolkit since it worked for you (as well as our tests).
Thank you! -Kainen
I can't find this in the installation instructions on github or readthedocs.
jacobblum
commented
1 year ago @djps
@jacobblum - I can't seem to install it when following the instructions: In windows 11, using conda 23.5.2, (with python 3.11), cuda driver (528.90) and nvcc 11.3.58 but can't install the cuda-toolkit. It finds lots of conflicts which it can't resolve. What version of conda was this tested on? I had tried different channels also. I was able to install it using pip though. The tests passed after 28 minutes. I could replicate the quick start examples as well.
Hi @djps, We are looking more into what may have caused the errors you experienced when using the installation instructions we provided. At present, we have been unable to replicate the error -- we will get back to you with further clarifying questions should we need more information. In the meantime, we will update the installation instructions to use pip instead of conda to install cudatoolkit since it worked for you (as well as our tests). Thank you! -Kainen
I can't find this in the installation instructions on github or readthedocs.
Instead of changing the numba/cudatoolkit installation to using pip, we decided to clarify in the installation instructions that the specific numba and cudatoolkit installation may vary, and what's shown is what worked on our x64 windows 10 machines. In particular, the installation instructions prompt users to...
" install numba by following the linked instructions. For different hardware platforms, the specific numba installation syntax may varry. These instructions are covered in the numba installation guide. Shown below are the commands for installation on our lab's computers, which are x64-based windows machines. "
The numba documentation covers the use of pip for installation, which you noted was successful in your case.
Thanks, Jacob
djps
commented
1 year ago @djps
@jacobblum - I can't seem to install it when following the instructions: In windows 11, using conda 23.5.2, (with python 3.11), cuda driver (528.90) and nvcc 11.3.58 but can't install the cuda-toolkit. It finds lots of conflicts which it can't resolve. What version of conda was this tested on? I had tried different channels also. I was able to install it using pip though. The tests passed after 28 minutes. I could replicate the quick start examples as well.
Hi @djps, We are looking more into what may have caused the errors you experienced when using the installation instructions we provided. At present, we have been unable to replicate the error -- we will get back to you with further clarifying questions should we need more information. In the meantime, we will update the installation instructions to use pip instead of conda to install cudatoolkit since it worked for you (as well as our tests). Thank you! -Kainen
I can't find this in the installation instructions on github or readthedocs.
Instead of changing the numba/cudatoolkit installation to using pip, we decided to clarify in the installation instructions that the specific numba and cudatoolkit installation may vary, and what's shown is what worked on our x64 windows 10 machines. In particular, the installation instructions prompt users to...
" install numba by following the linked instructions. For different hardware platforms, the specific numba installation syntax may varry. These instructions are covered in the numba installation guide. Shown below are the commands for installation on our lab's computers, which are x64-based windows machines. "
The numba documentation covers the use of pip for installation, which you noted was successful in your case.
Thanks, Jacob
ah ok, i was refering to the statement by @kainenutt regarding updating the installation instructions "to use pip instead of conda to install cudatoolkit since it worked for you (as well as our tests)."
mfroeling
commented
1 year ago @jacobblum @jgostick, I'm happy with the more nuanced and in depth statement of need. With these last adjustments I think all requirements are met so i have checked all boxes.
Looking forward to the evolution of this tool, maybe also allow for some realistic muscle geometries with membrane permeability, then you really get me exited ;D.
jgostick
commented
1 year ago @djps Can you confirm if you're happy with the last two check-boxes?
djps
commented
1 year ago @djps Can you confirm if you're happy with the last two check-boxes?
- Installation: Does installation proceed as outlined in the documentation?
- Functionality: Have the functional claims of the software been confirmed?
A one sentence regarding installation using pip would be good, but I am not going to insist on it.
jacobblum
commented
1 year ago @djps Can you confirm if you're happy with the last two check-boxes?
- Installation: Does installation proceed as outlined in the documentation?
- Functionality: Have the functional claims of the software been confirmed?
A one sentence regarding installation using pip would be good, but I am not going to insist on it.
@djps
Thank you for the feedback. We'll add a troubleshooting section to the installation section and mention alternative installation of Numba via pip. I was able to test this out and confirmed it worked on our machines over the weekend.
We've also addressed the following items in the manuscript.
There is duplication in the captions of the figures, so they read "Figure 1: Figure 1" The capitalisation of accronyms is inconsistent in lines 6 and 7. On line 39 it should read $\mu$m
jacobblum
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
jacobblum
commented
1 year ago @djps
Review checklist for @djps
Functionality
- [ ] Installation: Does installation proceed as outlined in the documentation?
Please see our troubleshooting section (https://simdrift.readthedocs.io/en/latest/troubleshooting/index.html) that walks through installation with pip. We have also amended the main installation instructions to include, in the numba step:
If you encounter any difficulties with this step, please see our troubleshooting guide for an alternative installation procedure.
We hope this satisfies this element of the checklist, and if not please let us know what additional steps you'd like us to take. Thanks so much!
jacobblum
commented
1 year ago @jgostick It looks like all of the checklist items are now complete for both reviewers. How should we proceeded? Thanks so much!
Also, @mfroeling and @djps, thank you both so much for your helpful comments throughout the review processes, we really appreciated your feedback!
jgostick
commented
1 year ago Hi @jacobblum, yes you are correct, the review looks complete. Congrats to you and thank you very much to the reviewers for donating their time to keep the JOSS machine running!
There are some steps we need to take to finalize this. I will create another checklist of items for you and I to address.
jgostick
commented
1 year ago @editorialbot set <DOI here> as archive@editorialbot set <version here> as version@editorialbot generate pdf@editorialbot check references and ask author(s) to update as needed@editorialbot recommend-acceptjacobblum
commented
1 year ago @editorialbot generate pdf
editorialbot
commented
1 year ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
jacobblum
commented
1 year ago @jgostick
Thank you for the message. Everything should be set for the author tasks.
I've added a new release version 1.0.13 with a Git tag of v1.0.13
and added the repository to Zenodo. The DOI is 10.5281/zenodo.8351982
Thanks!
jgostick
commented
1 year ago @editorialbot set 1.0.13 as version
editorialbot
commented
1 year ago Done! version is now 1.0.13
jgostick
commented
1 year ago @editorialbot set 10.5281/zenodo.8351982 as archive
Submitting author: !--author-handle-->@jacobblum<!--end-author-handle-- (Jacob Blum) Repository: https://github.com/jacobblum/simDRIFT Branch with paper.md (empty if default branch): main Version: 1.0.13 Editor: !--editor-->@jgostick<!--end-editor-- Reviewers: @mfroeling, @djps Archive: 10.5281/zenodo.8351982
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@mfroeling & @djps, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jgostick know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @djps
📝 Checklist for @mfroeling