editorialbot
commented
2 years ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Closed editorialbot closed 2 years ago
editorialbot
commented
2 years ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
vipinagrawal25
commented
2 years ago @fboehm, We have finished the review. We thank the referees, @mgiardino and @victorapm, for the generous comments. We have taken care of most of them. The comments have significantly improved the quality of the code. Below, we summarize the changes that we have made in our package as well as in the paper.
Consider fixing the compilation warnings listed below: src/init.cpp: In function ‘void init_read_parameters(MBRANE_para*, AFM_para*, MCpara*, char*)’: src/init.cpp:102:37: warning: format ‘%d’ expects a matching ‘int*’ argument [-Wformat=] ...
We have fixed all the compilation warnings in the current version. We have checked that the fix works with compiler version gcc-5.4 and gcc-12.1.
Is there a reason to use 60000 MC steps by default? Should this parameter be accessible to the user in the exe_start driver?
Thank you for the suggestion. In the current version, we have made this parameter accessible
to the user. The user can input this parameter as the fourth command line argument
to exe_start driver.
Code organization:
Consider generating a library so that external application codes can link to it. Consider moving the bulk of the work performed in the drivers start.cpp and memc.cpp to separate functions located at the src/ folder. This way, you can create a library that distributes those functionalities to external users.
The code was already organized in the suggested manner. All the functions are
defined in the files stored in src/ folder and only the function calls are done
from start.cpp and memc.cpp. Since the compilation time itself of our package is
very small, we have not considered generating a library. As we develop further and
the code grows we will surely make a library for the functions in src/.
I could not find API documentation anywhere. For example, what are the meanings of the input/output parameters used in functions such as monte_carlo_surf2d, pairlj_total_energy, and monte_carlo_3d? Consider using a documentation generator tool such as Doxygen. Note that functions should be annotated to be able to automatically generate the documentation.
We have generated the documentation using Doxygen and is now hosted at vipinagrawal25.github.io/MeMC. We have mentioned this in readme as well.
Add a make clean target (could be the same as make distclean).
We have added the make clean target in the Makefile to remove all the build objects and executables.
Code performance: Have you considered multithreading, e.g., via OpenMP, to speedup functions that take most of the execution time? For example, Gprof indicates that cal_length takes up to 70% of the execution time of exe_start and bending_energy_ipart takes up to 90% of the execution time of exe_memc. Would it be possible to reduce the execution time of these particular functions?
We thank the referee for this suggestion. Regarding optimizing exe_memc: we are able to optimize the bending_energy_ipart further by vectorizing some operations and reduce the compute time of bending_energy_ipart by 4%.
However, this routine is called atleast 6 times more than any other functions,
and the number of floating-point operations in the function is large.
It is natural to expect this function to be a major bottleneck in the code.
The current profile report is included in the repository (paper/prof_report.txt).
The number of calls to cal_length in exe_start is considerably large. Therefore, we expect this function to be the bottleneck too. However, the sole purpose of exe_start is to generate initial configuration to be fed into exe_memc. A single simulation can generate multiple such configurations and the user need not execute exe_start again. Thus, optimizing this driver is not so urgent.
We agree with the referee that openMP parallelization can be helpful. However, in the Monte-Carlo simulations, the parallelization is not immediately obvious as only one particle can be moved randomly at a given instance. During parallelization it may happen that multiple moves are made simultaneously. For this reason, we have postponed openMP parallelization till careful consideration. We have added the suggestion in the todo list and will implement them in future.
Minor changes in the text: Lines 60-61: did you mean: there are two elastic constants to be determined? Can you elaborate on this statement?
They are not to be determined but they are input parameters. We have clarified this in the text lines 61--63.
Line 64: should the definition of R be moved closer to its first appearance, i.e., closer to Equation 3?
Yes. We now define R below equation 3.
What is the meaning of K_B in Equation 6?
k_B is the Boltzmann constant (1.380649 × 10-23 SI units). We have also added the definition in the text.
What is the meaning of H in Equation 9?
H is the spring constant of the individual bonds between the nodes. Itzykson (see the paper cited in line 229) has shown that for a randomly triangulated membrane, the Young's modulus Y=2H/sqrt{3}.
Lines 110-113, 119, 127: which subsubsection are you talking about? I cannot see the numbers.
In the earlier version, the referencing was done incorrectly. Now we have fixed it.
In Equation 17, how \Delta V is computed? Is it simply V - V_0?
No, it is not. We have now added a paragraph below equation (17) to clarify this confusion.
Line 121-122: remove the text ::: subequations :::
Done.
Add community guidelines which should contain information about how to: Contribute to the software.
Report issues or problems with the software. Seek support.
We have added the following to the text: How to contribute The code is licensed under GPL-3.0 and hosted at here. For any contribution, the developer can send a pull request. For any queries, the user should open an issue on github.
Add white background to the figure: https://github.com/vipinagrawal25/MeMC/blob/main/paper/fig/describe_theta.png.
We have now made the background white.
Update links in https://github.com/vipinagrawal25/MeMC/blob/main/README.md#example gnuplot: http://www.gnuplot.info/ GNU GSL: https://www.gnu.org/software/gsl/
We have fixed all the links
Statement of need The target audience is somewhat implicit.
We have added the following in the text: Our software is targeted towards physicists and biologists working in soft matter.
References The (n.d. …) style references are syntactically correct?
We have now fixed it.
Functionality/Performance No performance claims.
We have not tested the code in various platform to comment on the performance.
fboehm
commented
2 years ago Thank you, @thecalculon and @vipinagrawal25 ! @victorapm and @mgiardino - are you satisfied with the fixes that the authors have implemented? Please comment here. Once you're satisfied with the submission, we'll proceed towards publication.
victorapm
commented
2 years ago @vipinagrawal25, thanks for addressing my points and congratulations again for your nice work!
@fboehm I'm satisfied with the changes implemented by the authors. Thanks for the opportunity in reviewing this work and please let me know about other opportunities...
mgiardino
commented
2 years ago @fboehm I'm satisfied with the changes implemented by the authors, I think they've been very solicitous to our comments. Many thanks for this opportunity, I hope that my contribution was helpful to improve their nice work.
fboehm
commented
2 years ago Thanks so much, @victorapm and @mgiardino! I really appreciate the thorough reviews, and I hope that we can work together on future submissions. @vipinagrawal25 - the reviewers have recommended your submission for publication. The next step is for me to review the manuscript. I may offer minor corrections to it before we proceed. You can expect to hear from me in the next few days.
fboehm
commented
2 years ago @editorialbot check references
editorialbot
commented
2 years ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acsnano.6b07302 is OK
- 10.1073/pnas.1212268109 is OK
- 10.1103/physrevx.7.011002 is OK
- 10.1103/physrevlett.82.221 is OK
- 10.1007/s101890170071 is OK
- 10.1103/physreve.72.031904 is OK
- 10.1103/physrevx.7.011002 is OK
- 10.1007/978-3-642-13193-6_39 is OK
- 10.1529/biophysj.106.081422 is OK
- 10.1103/physreve.78.051924 is OK
- 10.1103/physreva.38.1005 is OK
- 10.1007/978-3-662-05105-4 is OK
- 10.1007/978-3-662-05105-4_2 is OK
- 10.1021/acssensors.9b00418.s001 is OK
MISSING DOIs
- 10.1073/pnas.202617299 may be a valid DOI for title: Stomatocyte–discocyte–echinocyte sequence of the human red blood cell: Evidence for the bilayer–couple hypothesis from membrane mechanics
INVALID DOIs
- None@vipinagrawal25 - can you examine the above output from the check references command? Is the suggested "missing doi" accurate? If so, would you please update your manuscript to include it?
thecalculon
commented
2 years ago @editorialbot check references
editorialbot
commented
2 years ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acsnano.6b07302 is OK
- 10.1073/pnas.1212268109 is OK
- 10.1103/physrevx.7.011002 is OK
- 10.1073/pnas.202617299 is OK
- 10.1103/physrevlett.82.221 is OK
- 10.1007/s101890170071 is OK
- 10.1103/physreve.72.031904 is OK
- 10.1103/physrevx.7.011002 is OK
- 10.1007/978-3-642-13193-6_39 is OK
- 10.1529/biophysj.106.081422 is OK
- 10.1103/physreve.78.051924 is OK
- 10.1103/physreva.38.1005 is OK
- 10.1007/978-3-662-05105-4 is OK
- 10.1007/978-3-662-05105-4_2 is OK
- 10.1021/acssensors.9b00418.s001 is OK
MISSING DOIs
- None
INVALID DOIs
- None@vipinagrawal25 - can you examine the above output from the check references command? Is the suggested "missing doi" accurate? If so, would you please update your manuscript to include it?
@fboehm the doi suggeted by the 'good bot' was correct. We have updated it in the paper.
fboehm
commented
2 years ago Excellent! Thanks, @thecalculon and @vipinagrawal25 !
fboehm
commented
2 years ago @editorialbot generate pdf
editorialbot
commented
2 years ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
fboehm
commented
2 years ago Here are a few small suggestions for the text of the pdf:
fboehm
commented
2 years ago the doi for Gompper & Kroll resolves to a reference other than that listed. Can you fix this?
fboehm
commented
2 years ago @vipinagrawal25 @thecalculon - the other dois, excepting Gompper and Kroll, resolve to the intended targets.
vipinagrawal25
commented
2 years ago @editorialbot check references
editorialbot
commented
2 years ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acsnano.6b07302 is OK
- 10.1073/pnas.1212268109 is OK
- 10.1103/physrevx.7.011002 is OK
- 10.1073/pnas.202617299 is OK
- 10.1103/physrevlett.82.221 is OK
- 10.1007/s101890170071 is OK
- 10.1103/physreve.72.031904 is OK
- 10.1103/physrevx.7.011002 is OK
- 10.1007/978-3-642-13193-6_39 is OK
- 10.1529/biophysj.106.081422 is OK
- 10.1103/physreve.78.051924 is OK
- 10.1103/physreva.38.1005 is OK
- 10.1007/978-3-662-05105-4 is OK
- 10.1007/978-3-662-05105-4_2 is OK
- 10.1021/acssensors.9b00418.s001 is OK
- 10.1021/jp054372b is OK
MISSING DOIs
- None
INVALID DOIs
- None@fboehm Thanks for your comments. DOIs and the text are fixed now.
thecalculon
commented
2 years ago @editorialbot generate pdf
editorialbot
commented
2 years ago I'm sorry human, I don't understand that. You can see what commands I support by typing:
@editorialbot commands
thecalculon
commented
2 years ago @editorialbot generate pdf
fboehm
commented
2 years ago @editorialbot generate pdf
editorialbot
commented
2 years ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
fboehm
commented
2 years ago The Gomper & Kroll ref's doi now resolves to the intended target.
fboehm
commented
2 years ago @vipinagrawal25 - the pdf looks good. Please make a release and archive the repository, for example, with zenodo. Then, report here the release version number and the archive doi. Thank you!
vipinagrawal25
commented
2 years ago @vipinagrawal25 - the pdf looks good. Please make a release and archive the repository, for example, with zenodo. Then, report here the release version number and the archive doi. Thank you!
The details of the released version from Zenodo are as follows: Agrawal Vipin, Pandey Vikash, Kylhammar Hanna, Dev Apruba, & Mitra Dhrubaditya. (2022). MeMC: A package for Monte Carlo simulations of spherical shells (v1.1). Zenodo. https://doi.org/10.5281/zenodo.6671531
fboehm
commented
2 years ago @editorialbot set 10.5281/zenodo.6671531 as archive
editorialbot
commented
2 years ago Done! Archive is now 10.5281/zenodo.6671531
fboehm
commented
2 years ago @editorialbot set v1.1 as version
editorialbot
commented
2 years ago Done! version is now v1.1
fboehm
commented
2 years ago @vipinagrawal25 - I'm looking at the archive. It seems that you've reversed the ordering of names for the authors, where you list the family name before the first name. we need the names to match exactly those in the paper.md. Can you fix this?
vipinagrawal25
commented
2 years ago @vipinagrawal25 - I'm looking at the archive. It seems that you've reversed the ordering of names for the authors, where you list the family name before the first name. we need the names to match exactly those in the paper.md. Can you fix this?
We have fixed the name ordering.
fboehm
commented
2 years ago @editorialbot recommend-accept
editorialbot
commented
2 years ago Attempting dry run of processing paper acceptance...Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1021/acsnano.6b07302 is OK
- 10.1073/pnas.1212268109 is OK
- 10.1103/physrevx.7.011002 is OK
- 10.1073/pnas.202617299 is OK
- 10.1103/physrevlett.82.221 is OK
- 10.1007/s101890170071 is OK
- 10.1103/physreve.72.031904 is OK
- 10.1142/9789812565518_0012 is OK
- 10.1007/978-3-642-13193-6_39 is OK
- 10.1529/biophysj.106.081422 is OK
- 10.1103/physreve.78.051924 is OK
- 10.1103/physreva.38.1005 is OK
- 10.1007/978-3-662-05105-4 is OK
- 10.1007/978-3-662-05105-4_2 is OK
- 10.1021/acssensors.9b00418.s001 is OK
- 10.1021/jp054372b is OK
MISSING DOIs
- None
INVALID DOIs
- None:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.
Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/3291
If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3291, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept
Kevin-Mattheus-Moerman
commented
2 years ago I have checked the archive and meta-data, and the version tag. I have also briefly reviewed the paper and all seems in order. I will now proceed to process acceptance.
Kevin-Mattheus-Moerman
commented
2 years ago @editorialbot accept
editorialbot
commented
2 years ago Doing it live! Attempting automated processing of paper acceptance...🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦
editorialbot
commented
2 years ago 🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨
Here's what you must now do:
Any issues? Notify your editorial technical team...
Kevin-Mattheus-Moerman
commented
2 years ago @vipinagrawal25 congratulations on getting this paper published in JOSS!
@fboehm thanks for editing this one!
And special thanks to @victorapm and @mgiardino for your review work!! :tada:
editorialbot
commented
2 years ago :tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:
If you would like to include a link to your paper from your README use the following code snippets:
Markdown:
[](https://doi.org/10.21105/joss.04305)
HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.04305">
<img src="https://joss.theoj.org/papers/10.21105/joss.04305/status.svg" alt="DOI badge" >
</a>
reStructuredText:
.. image:: https://joss.theoj.org/papers/10.21105/joss.04305/status.svg
:target: https://doi.org/10.21105/joss.04305This is how it will look in your documentation:
We need your help!
The Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
Submitting author: !--author-handle-->@vipinagrawal25<!--end-author-handle-- (Vipin Agrawal) Repository: https://github.com/vipinagrawal25/MeMC Branch with paper.md (empty if default branch): Version: v1.1 Editor: !--editor-->@fboehm<!--end-editor-- Reviewers: @victorapm, @mgiardino Archive: 10.5281/zenodo.6671531
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
@victorapm & @mgiardino, 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 @fboehm 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 @victorapm
📝 Checklist for @mgiardino