Closed editorialbot closed 9 months 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.13 s (788.7 files/s, 149178.6 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
C++ 18 2790 1365 6812
Bourne Shell 32 823 220 2360
Fortran 90 11 619 210 1254
C/C++ Header 18 513 526 671
Markdown 15 192 0 443
TeX 2 13 0 181
Python 2 78 70 149
YAML 2 13 4 56
MATLAB 1 24 2 31
Solidity 1 14 0 30
make 1 4 7 9
-------------------------------------------------------------------------------
SUM: 103 5083 2404 11996
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryReference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1063/1.1563248 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.1021/acs.jpca.2c00604 is OK
- 10.1063/5.0028219 is OK
- 10.1038/124119a0 is OK
- 10.1063/1.445633 is OK
- 10.1063/1.449884 is OK
- 10.1103/PhysRevLett.85.4751 is OK
- 10.1103/physrevlett.94.086102 is OK
- 10.1063/1.1563248 is OK
- 10.1103/physrevlett.100.036104 is OK
- 10.1021/ct300193e is OK
- 10.1016/j.commatsci.2018.08.035 is OK
- 10.1016/j.commatsci.2013.03.018 is OK
- 10.1063/1.4967521 is OK
- 10.1063/5.0028653 is OK
MISSING DOIs
- None
INVALID DOIs
- NoneWordcount for paper.md is 1419
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:
mbarzegary
commented
1 year ago @ptmerz @alexgorfer thanks for your help reviewing this work! This is where the review happens. I kindly ask you to now formally start the review. Follow the instructions above ☝️ to generate a check box list for yourself here to guide you through the process. Let me know if you have any questions. Thanks again!
mbarzegary
commented
1 year ago @demonico85 @lorenzo-rovigatti this is where the review takes place. Please keep an eye out for comments here from the reviewers, as well as any issues opened by them on your software repository. I recommend you aim to respond to these as soon as possible, and you can address them straight away as they come in if you like, to ensure we do not loose track of the reviewers.
alexgorfer
commented
1 year ago 
ptmerz
commented
1 year ago alexgorfer
commented
1 year ago Hey @demonico85, I have a small problem with getting the test to work. I made an issue on the repository (https://github.com/demonico85/cleaving/issues/6)
ptmerz
commented
1 year ago Confirming that the test does not work for me either, I added a note at the issue above.
ptmerz
commented
1 year ago I am also wondering if you could comment on your choice of distribution / installation method. Why did you decide against contributing your work to LAMMPS directly? I think that would have many benefits, including a review from the LAMMPS maintainers, reducing the chance of integration problems (for the current or a future version), as well as a simpler installation process for users. Downloading a third-party repo and copying source files into the LAMMPS distribution certainly works, but isn't the greatest user experience, and might be especially challenging when trying to use the package on HPC resources.
ptmerz
commented
1 year ago Another point Re: "Contribution and authorship: Has the submitting author (@demonico85) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?". It's easy to verify through the git log that @demonico85 and @lorenzo-rovigatti (first and last author) have contributed significantly to the software. The git log does not allow to verify Ruslan Davidchacks contribution. Can you comment on including them as an author?
ptmerz
commented
1 year ago Further points Re: "Documentation" in the checklist above:
I'd like to note that the documentation of the different LAMMPS commands is excellent. I really appreciate the work you put into explaining each command in depth, often including formulas, references and plots.
ptmerz
commented
1 year ago Here's a few more things I noticed:
utils/ folder, which make up a non-trivial part of your source code. Unfortunately, most of these are not documented. I see that 3 of them are described in the last step of the examples, but one of them (calcSFE.m) isn't even present in my checkout. This leaves 40 files without any description. Do you consider these files part of this submission? In this case, I think these would need a description in the documentation, and ideally tests.I hope that all the points that I raised above don't come over as if I was thinking negatively of the submission. In fact, I think that the work is highly relevant, the LAMMPS functions are nicely documented, and the paper is very well written.
mbarzegary
commented
1 year ago Thank you @ptmerz for the review. @demonico85 and @lorenzo-rovigatti you can start addressing the issues mentioned above.
lorenzo-rovigatti
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:
lorenzo-rovigatti
commented
1 year ago Thanks a lot for your thorough review, @ptmerz ! We are going through all the points you have raised. For now we have fixed the paper, added a statement of need to the docs (in the form of a "background" section) and enforced using CLEAVING throughout the docs. I'll update the following list as soon as we are done with each item:
We will provide answers (and fixes) to the issues created in the repo directly there!
alexgorfer
commented
1 year ago Thank you for fixing https://github.com/demonico85/cleaving/issues/6 so quickly! I went through testing again and then the examples and it all worked perfectly!
alexgorfer
commented
1 year ago The work implements an accessible cleaving method in LAMMPS which will certainly be of interest for the readership of JOSS. Overall the work is done carefully, the documentation is clearly written and figures are informative. Adding to what has been put forth ptmerz already, here are some additional comments:
Functionality The main point that I would like discussed in more depth is the range of applicability of the software. What I mean is that in the documentation and paper we learn about prior uses of the method including hard and soft-spheres, LJ, TIP4P, EAM and others and that the adoption for different forcefields requires a more convenient simulation interface. Which is the motivation for this work. To me this suggested that the software can already cleave TIP4P, EAM and others, but I am not sure if this is the case since in TIP4P, Handel and Davichack et al. developed an orientational cleaving potential for step 2, and in the case of EAM, Liu and coworkers added a soft repuslive part to the potential in order to compensate their calculation at non-coexistence, again to achieve crystal layers in step 2. Since I do not see this implemented, could you clarify in the documentation and paper what the current implementation is capable of? Say I want to repeat Handel et al.s work, can I:
If the latter is the case and its not possible to do step 2 for arbitrary systems, is it still possible to do step 1,3 and 4? So the solid-vacuum TIP4P interface. TIP4P can be made with lj/cut/coul and fix rigid and if that all works with your implementation it would be awesome to add an example of this more complicated scenario.
Minor points:
mbarzegary
commented
1 year ago @demonico85 @lorenzo-rovigatti please can you provide an update on the above, in terms of responding to the issues raised? It would be good to respond to issues raised in a timely manor, to avoid delays and to avoid loosing track of reviewers.
demonico85
commented
1 year ago @demonico85 @lorenzo-rovigatti please can you provide an update on the above, in terms of responding to the issues raised? It would be good to respond to issues raised in a timely manor, to avoid delays and to avoid loosing track of reviewers.
My apologies for the long delay in the replies due to personal issues. We will resume the discussion and replies to the reviewers in the next few days
demonico85
commented
1 year ago I am also wondering if you could comment on your choice of distribution / installation method. Why did you decide against contributing your work to LAMMPS directly? I think that would have many benefits, including a review from the LAMMPS maintainers, reducing the chance of integration problems (for the current or a future version), as well as a simpler installation process for users. Downloading a third-party repo and copying source files into the LAMMPS distribution certainly works, but isn't the greatest user experience, and might be especially challenging when trying to use the package on HPC resources.
We agree with the reviewer's comment about the less convenient user experience in having a separate package with respect to an addition to the main LAMMPS repository. However, we did not decide against the inclusion in LAMMPS directly. According to LAMMPS developers (https://docs.lammps.org/Modify_contribute.html), external packages are equally welcomed to increase the potentiality of LAMMPS itself. Therefore, we decided, for the moment, to stick with this form of contribution rather than submitting the package to the LAMMPS developer, creating a package with the core code needed to run the cleaving model. Future extensions of this core model with different type of interactions (e.g., those used in water model) will make the package general enough for us to consider to request to LAMMPS developer to be included in the main distribution
demonico85
commented
1 year ago Another point Re: "Contribution and authorship: Has the submitting author (@demonico85) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?". It's easy to verify through the git log that @demonico85 and @lorenzo-rovigatti (first and last author) have contributed significantly to the software. The git log does not allow to verify Ruslan Davidchacks contribution. Can you comment on including them as an author?
Most of the code we are presenting here, which was developed in this form mainly by myself and @lorenzo-rovigatti is based on a in-house Fortran code written by the third author: Ruslan Davidchack. For this reason, despite the obvious differences (Fortran VS c++, in-house MD code with cleaving VS package for LAMMPS) the contribution of the third author to the code is substantial, even if he is not listed as author of the repository itself. In our opinion, and for the sake of fairness, we feel that he needs to be included as an author of the repository
demonico85
commented
1 year ago Here's a few more things I noticed:
- utils: You have a large number of files in the
utils/folder, which make up a non-trivial part of your source code. Unfortunately, most of these are not documented. I see that 3 of them are described in the last step of the examples, but one of them (calcSFE.m) isn't even present in my checkout. This leaves 40 files without any description. Do you consider these files part of this submission? In this case, I think these would need a description in the documentation, and ideally tests.
The utils/ folder contains a miscellanea of programs that can help in working with the cleaving model but they are not part of the model itself (e.g., bash files to run the LAMMPS simulations on a queue system). We included them to avoid the users to write their own short scripts (e.g., to collect the work in step3) if they want, but since this is not part of the cleaving model we did not include any explanation in the documentation. The reason is that if a user finds it easier to write a bash script using awk to extract the work in step3, rather than adapting the fortran code provided in the /utils folder, that's perfectly reasonable.
As per the calcSFE.m subroutine, we corrected the description of the guide related to the examples. That particular subroutine is located in the folder /examples/LJ_SL/utils (for the example on solid-liquid interface) and in the folder /examples/LJ_SV/utils (for the example on solid-vacuum interface). We wanted to make the example as self-contained as possible, so each of them has their own post-processing subroutines (adapted for that particular example).
However, we agree that it maybe confusing to have a bunch of code without any explanation at all. We included in the guide a list of the subroutines in utils/ with a single explanation line to help the user to navigate through them.
lorenzo-rovigatti
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:
demonico85
commented
1 year ago The work implements an accessible cleaving method in LAMMPS which will certainly be of interest for the readership of JOSS. Overall the work is done carefully, the documentation is clearly written and figures are informative. Adding to what has been put forth ptmerz already, here are some additional comments:
Functionality The main point that I would like discussed in more depth is the range of applicability of the software. What I mean is that in the documentation and paper we learn about prior uses of the method including hard and soft-spheres, LJ, TIP4P, EAM and others and that the adoption for different forcefields requires a more convenient simulation interface. Which is the motivation for this work. To me this suggested that the software can already cleave TIP4P, EAM and others, but I am not sure if this is the case since in TIP4P, Handel and Davichack et al. developed an orientational cleaving potential for step 2, and in the case of EAM, Liu and coworkers added a soft repuslive part to the potential in order to compensate their calculation at non-coexistence, again to achieve crystal layers in step 2. Since I do not see this implemented, could you clarify in the documentation and paper what the current implementation is capable of? Say I want to repeat Handel et al.s work, can I:
- use existing pair_styles and fixes (it would be great to have an example in this case)
- or do I need edit wellPforce to include orientation (add some documentation how to modify fixes for custom force-fields)
- or can I not use this package for this purpose yet (then state this more clearly)
If the latter is the case and its not possible to do step 2 for arbitrary systems, is it still possible to do step 1,3 and 4? So the solid-vacuum TIP4P interface. TIP4P can be made with lj/cut/coul and fix rigid and if that all works with your implementation it would be awesome to add an example of this more complicated scenario.
The reviewer is correct in pointing out the fact that the package does not include the capabilities to run the work of Handel et al.
The fix wellPforce does not include orientation
The pair styles currently considered in the repository do not include the TIP4P for water.
We have added the following sentences to the paper
Although the package in its current form does not include yet all the different interactions explored in the literature, it allows to reproduce the results in [@Davidchack03direct;@DiPasquale2022cleaving;@DiPasquale2020shuttleworth] and gives all the machinery needed to extend the calculations to more complex interaction models. The package we are presenting makes the addition of new interactions type a straightforward extension of the code already given here.
to make clear what the package can achieve at the moment, after listing the results obtained with the cleaving model. In our opinion, it is still an important piece of information showing the capabilities and possible range of application of the cleaving model, to advocate for the importance of the current repository.
Having said that, the important concept for our repository is that it represents the core functionalities on top of which more complicated models can be built on. Step 1, 2, 4 can be performed for arbitrary interactions, because what is needed for the cleaving are the external potentials (walls or wells). Instead, for step3 some modifications of the subroutines calculating the interactions are needed. The machinery for step3 to switch on/off the interactions is already in the repository (i.e., the move/dupl, duplicate), but a new interaction style not already included in the repository would need to be modified to include the scaling of the interactions with lambda (as done e.g., for pair_lj_BGcleav.cpp). However, these modifications just require adapting the already existing subroutines to the new interactions one wishes to consider. The repository as it stands at the moment allows either to run directly the cleaving model with LAMMPS (with a limited selection of interactions, LJ, Broughton and Gilmer LJ, Fennel potential (dsf)) or to provide a firm basis for extension to more complicated/exotic interactions.
As a side note, we are already working in extending the cleaving repository with TIP4P interactions. As we are still testing the results, we put them in a separate branch (graphene) of the repository. They will become part of the repository as soon as they are ready.
We want to highlight here that this last remark is important to see the importance of what we are presenting at the moment. Extensions of the repository will include more different interactions, which will be added in the future, but the core functionalities for the cleaving model (on top of which everything else can be built) are the one we are presenting with this submission.
Minor points:
- In evaluating step3 of examples/lj_SV using a.out I got ERROR file zdir.dat not found. I copied zdir.dat into /out/ and it worked. All other steps in the example worked without issue!
Corrected the documentation, the file zdir.dat must be copied in the dir from where a.out is called
- Does it have to be Matlab to calculate the integrals? Something non-proprietary would be preferable, like the calcintegrals.f90 in utils.
No, Matlab is not required. It is only a numerical calculation of an integral, for the sake of the cleaving model it does not matter the way it is performed. We included the matlab file as it was the one we used to derive the results published in the paper on which the example are based [Di Pasquale & Davidchack 2020]
- Speaking of utils, there is no documentation for these scripts but some sound very useful for users.
We now included in the documentation a brief description of each subroutine in the utils folder. As we discussed in another reply, since these subroutines are not part of the cleaving model (they are basically pre/post processing) we did not want to "force" them on the user. For example, if Matlab is not an option, any python/fortran/excel/... subroutine for the calculation of the integral is ok
- The citation style on line 68-73 is not uniform (author, year) vs author (year). Also dont capitalize thermodynamic integration after defining the acronym.
fixed the citation style
lorenzo-rovigatti
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:
alexgorfer
commented
1 year ago Ok perfect thanks for clarifiying this in more depth! That concludes all the points I raised so from my side the work can go ahead.
lorenzo-rovigatti
commented
1 year ago We have added a new testing script that runs short cleaving steps and outputs the resulting work. The numbers can be used to check for any regression bugs, as suggested by @ptmerz . We have updated the documentation to reflect this change. We have also added contributing guidelines (both to the readme and to the docs), as well as added a background/statement of need to the docs. If I'm not mistaken these were the last things that we were asked to discuss/change!
mbarzegary
commented
11 months ago @ptmerz can you please have a look at the comments above and check if they address your concerns?
ptmerz
commented
11 months ago Apologies for the delay, I missed the updates here. I'll get back to this this week.
mbarzegary
commented
10 months ago Hi @ptmerz, can you please update us on your review by checking the authors' response to your concerns?
ptmerz
commented
10 months ago I first want to thank the authors for the improvements they have implemented in response to the reviewer comments. I have finished the outstanding review tasks to complete the checklist, and I think the work presented here is relevant to the field and the software quality is good, making it overall well suited for JOSS.
lorenzo-rovigatti
commented
9 months ago Thank you @ptmerz ! @mbarzegary I think we are good to go?
mbarzegary
commented
9 months ago Thank you @alexgorfer @ptmerz for the review. @lorenzo-rovigatti yes, I'll get back to you soon.
mbarzegary
commented
9 months ago @editorialbot generate pdf
editorialbot
commented
9 months ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
mbarzegary
commented
9 months ago @editorialbot check references
editorialbot
commented
9 months ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1063/1.1563248 is OK
- 10.1016/j.cpc.2021.108171 is OK
- 10.1021/acs.jpca.2c00604 is OK
- 10.1063/5.0028219 is OK
- 10.1038/124119a0 is OK
- 10.1063/1.445633 is OK
- 10.1063/1.449884 is OK
- 10.1103/PhysRevLett.85.4751 is OK
- 10.1103/physrevlett.94.086102 is OK
- 10.1063/1.1563248 is OK
- 10.1103/physrevlett.100.036104 is OK
- 10.1021/ct300193e is OK
- 10.1016/j.commatsci.2018.08.035 is OK
- 10.1016/j.commatsci.2013.03.018 is OK
- 10.1063/1.4967521 is OK
- 10.1063/5.0028653 is OK
MISSING DOIs
- None
INVALID DOIs
- None@lorenzo-rovigatti please merge my PR with some minor edits.
After that, could you:
I can then move forward with recommending acceptance of the submission.
lorenzo-rovigatti
commented
9 months ago Thank you @mbarzegary ! I merged the PR and uploaded the 1.0 version to Zenodo (DOI: 10.5281/zenodo.10567702),
mbarzegary
commented
9 months ago @lorenzo-rovigatti can you please make the license of the archive similar to the license of the software repository?
lorenzo-rovigatti
commented
9 months ago Good catch, sorry about that! I changed the license to GPL 3.0, which is the same we use for the code in the repo.
mbarzegary
commented
9 months ago @lorenzo-rovigatti good.
So, we are ready to proceed. I'm not sure if it's really an issue, but can you please change the authors' order in the archive to match the paper? Also, please modify the Zenodo listed version tag to read full v1.0 instead of v1.
After these modifications, I recommend acceptance of the paper.
mbarzegary
commented
9 months ago @editorialbot set 10.5281/zenodo.10567702 as archive
editorialbot
commented
9 months ago Done! archive is now 10.5281/zenodo.10567702
Submitting author: !--author-handle-->@demonico85<!--end-author-handle-- (Nicodemo Di Pasquale) Repository: https://github.com/demonico85/cleaving Branch with paper.md (empty if default branch): joss Version: v1.0 Editor: !--editor-->@mbarzegary<!--end-editor-- Reviewers: @ptmerz, @alexgorfer Archive: 10.5281/zenodo.10567702
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
@ptmerz & @alexgorfer, 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 @mbarzegary 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 @alexgorfer
📝 Checklist for @ptmerz