search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: Chitin Builder: a VMD tool for the generation of structures of chitin molecular crystals for atomistic simulations #5771

Closed editorialbot closed 10 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@jfaraudo<!--end-author-handle-- (JORDI FARAUDO) Repository: https://github.com/soft-matter-theory-at-icmab-csic/chitin_builder Branch with paper.md (empty if default branch): Version: v1.1 Editor: !--editor-->@majensen<!--end-editor-- Reviewers: @amoeba, @tonigi Archive: 10.5281/zenodo.3274725

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/c409d05d1e558827d1dea6275faf8965"><img src="https://joss.theoj.org/papers/c409d05d1e558827d1dea6275faf8965/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/c409d05d1e558827d1dea6275faf8965/status.svg)](https://joss.theoj.org/papers/c409d05d1e558827d1dea6275faf8965)

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

@amoeba & @WangKehan573, 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:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @majensen 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 @amoeba

📝 Checklist for @tonigi

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 commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.03 s (288.6 files/s, 64485.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
TeX                              1             50              0            690
Tcl/Tk                           2             73            124            647
Markdown                         4            100              0            258
Bourne Shell                     1              8             16             22
YAML                             1              1              4             18
-------------------------------------------------------------------------------
SUM:                             9            232            144           1635
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 3611

editorialbot commented 1 year ago

Failed to discover a Statement of need section in paper

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1021/ma0477246 is OK
- 10.1351/pac-rep-10-01-01 is OK
- 10.1021/bm201512t is OK
- 10.1016/j.cpc.2013.03.010 is OK
- 10.1021/ma0203849 is OK
- 10.1016/j.carbpol.2017.06.099 is OK
- 10.1115/1.4038883 is OK
- 10.1021/acs.biomac.5b01653 is OK
- 10.1021/bm801251e is OK
- 10.1007/s10570-016-0968-0 is OK
- 10.1016/0166-218x(88)90012-1 is OK
- 10.1038/nature23268 is OK
- 10.1103/physrevlett.120.068001 is OK
- 10.1371/journal.pone.0039376 is OK
- 10.1021/ma102240r is OK
- 10.1021/ma0477246 is OK
- 10.1016/S1381-5148(00)00038-9 is OK
- 10.1002/cben.201400025 is OK
- 10.1021/acssuschemeng.8b06372 is OK
- 10.1002/jcc.22959 is OK
- 10.1021/ct8002964 is OK
- 10.1021/ct200328p is OK
- 10.1081/MC-120006451 is OK
- 10.1351/pac-rep-10-01-01 is OK
- 10.1021/bm201512t is OK
- 10.1016/j.cpc.2013.03.010 is OK
- 10.1021/ma0203849 is OK
- 10.1016/j.carbpol.2017.06.099 is OK
- 10.1021/ct700301q is OK
- 10.1002/jcc.20291 is OK
- 10.1007/s12668-013-0097-2 is OK
- 10.1115/1.4038883 is OK
- 10.1021/acs.jpcb.6b05999 is OK
- 10.1007/s10853-015-9271-y is OK
- 10.1016/J.CIS.2019.02.003 is OK
- 10.1021/acs.biomac.5b01653 is OK
- 10.1016/J.CARBPOL.2017.08.076 is OK
- 10.1002/pola.20176 is OK
- 10.1021/bm801251e is OK
- 10.1007/s10570-016-0968-0 is OK
- 10.1002/jcc.20289 is OK
- 10.1016/0166-218x(88)90012-1 is OK
- 10.1039/C6GC00628K is OK
- 10.1039/C6RA00107F is OK
- 10.1038/nature23268 is OK
- 10.1103/physrevlett.120.068001 is OK
- 10.1016/0263-7855(96)00018-5 is OK
- 10.1371/journal.pone.0039376 is OK
- 10.1021/ma102240r is OK

MISSING DOIs

- None

INVALID DOIs

- None
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:

majensen commented 1 year ago

@amoeba, @WangKehan573, have you been able to give some time to this review? Please let me know in the thread. Thanks!

amoeba commented 1 year ago

Hi @majensen, I hadn't noticed this review go from pre-review to review so thanks for the ping! I can get my review done in the next two weeks.

amoeba commented 1 year ago

Review checklist for @amoeba

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

amoeba commented 1 year ago

Hi @majensen, while reviewing the "Substantial scholarly effort" checklist item, I find a could use some editorial help. Using the corresponding section in the Review criteria as a guide, I think this submission is borderline due to the size and apparent age of the codebase. I don't see that this was discussed in pre review but please let me know if I missed that

It seems to me the most important element of this checklist item is whether the work is of broad interest and likely to be cited which isn't something I can assess with my background. However, in the paper, the authors speak to this at the end of their Introduction and Statement of Need section and to me it sounds like they theorize this tool would be quite useful to others in their field. With that, I'm inclined to check "Substantial scholarly effort" so long as you agree.

majensen commented 1 year ago

Sorry for the long delay @amoeba and all. I am discussing this with one of the AEICs. @jfaraudo - can you comment here on your view of the effort put into this software, and on its usefulness to the community? I think it is a specialized community, and that is ok - how do you see this work contributing to the science?

majensen commented 1 year ago

@WangKehan573 - are you still able to review #5771? Please let me know if I can help- thanks

WangKehan573 commented 1 year ago

I regret to inform you that I am unable to review the material you have submitted due to circumstances beyond my control. Unfortunately, I am experiencing unforeseen challenges that are preventing me from fulfilling my responsibility as a reviewer. I apologize for any inconvenience this may cause and I assure you that I am doing my best to resolve the situation. I hope you can understand and forgive my inability to meet my commitments at this time. Thank you for your understanding

---- Replied Message ---- | From | Mark @.> | | Date | 11/02/2023 22:56 | | To | @.> | | Cc | @.>@.> | | Subject | Re: [openjournals/joss-reviews] [REVIEW]: Chitin Builder: a VMD tool for the generation of structures of chitin molecular crystals for atomistic simulations (Issue #5771) |

@WangKehan573 - are you still able to review #5771? Please let me know if I can help- thanks

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

jfaraudo commented 1 year ago

@majensen Yes, sure, we can comment on this here. Yes, the code is useful for a very specialized community. Our target are the VMD users that develop simulations of polymeric materials based on natural polymers. To make a comparison, it is an scope similar to that of the "cellulose builder" app, that generates structures cellulosic materials, but in our case for chitin based materials. The relevance of this is based on the fact that cellulose and chitin are the most abundant polymers in Earth and are both of natural origin and therefore interesting as "green" materials. They have also very interesting properties. Prediction by computer simulation of their properties and behaviour is relatively under-studied (compared with other less abundant materials or compared with other human-designed materials) due to practical difficulties in generating appropiate structures suitable for the simulations. The objective of this code is to solve this problem, providing interested researchers with a tool that is able to generate the necessary files to investigate by computer simulations the properties of materials based on different crsytall structures of chitin polymer. We hope that this app will help in the advancement of the understanding of chitin and rational design materials based on this natural polymer by computer simulation.

@jfaraudo - can you comment here on your view of the effort put into this software, and on its usefulness to the community? I think it is a specialized community, and that is ok - how do you see this work contributing to the science?

majensen commented 1 year ago

Thanks @jfaraudo - I have also spent some time getting a deeper understanding of the VMD framework, which it is very current use, with new plugins written for it frequently up to the present time (even though it was published first around 1996). @amoeba I would call it the "Emacs of Molecular Modeling". I have not heard any disagreement from the Associate Editor in Chief to this argument. So let's proceed with "Substantial scholarly effort".

majensen commented 1 year ago

@jfaraudo -- unfortunately one of our reviewers had to decline. I am sorry about the very long delay. Would you have any suggestions for reviewers with the right expertise who could fairly review your work? Much appreciated!

amoeba commented 1 year ago

So let's proceed with "Substantial scholarly effort".

Thanks for taking a look, @majensen. Works for me.

jfaraudo commented 1 year ago

Sorry for the delay in responding. I think apropiate referees could be developers of other VMD plugins. One example is Dr Toni Giorgino @tonigi [ https://github.com/tonigi | https://github.com/tonigi ] Let me think about others. jordi

From: "Mark Jensen" @.> To: "openjournals/joss-reviews" @.> Cc: "Jordi Faraudo" @.>, "Mention" @.> Sent: Thursday, 16 November, 2023 23:15:28 Subject: Re: [openjournals/joss-reviews] [REVIEW]: Chitin Builder: a VMD tool for the generation of structures of chitin molecular crystals for atomistic simulations (Issue #5771)

[ https://urldefense.com/v3/__https://github.com/jfaraudo__;!!D9dNQwwGXtA!SoVYcjZdOWxeXeN0LHD9qH4oQ6jNIU5hcjJUbM0yWsXZW_b9OuhoxRVV66y2u82gJIOXk1ZcVLfopsmGnCnIN695DQ$ | @jfaraudo ] -- unfortunately one of our reviewers had to decline. I am sorry about the very long delay. Would you have any suggestions for reviewers with the right expertise who could fairly review your work? Much appreciated!

— Reply to this email directly, [ https://urldefense.com/v3/__https://github.com/openjournals/joss-reviews/issues/5771*issuecomment-1815400010__;Iw!!D9dNQwwGXtA!SoVYcjZdOWxeXeN0LHD9qH4oQ6jNIU5hcjJUbM0yWsXZW_b9OuhoxRVV66y2u82gJIOXk1ZcVLfopsmGnCkvMKOaXw$ | view it on GitHub ] , or [ https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AHFLNXQOV2VH55UJH6W5273YE2GABAVCNFSM6AAAAAA3XYVVACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJVGQYDAMBRGA__;!!D9dNQwwGXtA!SoVYcjZdOWxeXeN0LHD9qH4oQ6jNIU5hcjJUbM0yWsXZW_b9OuhoxRVV66y2u82gJIOXk1ZcVLfopsmGnCkvfQuctQ$ | unsubscribe ] . You are receiving this because you were mentioned. Message ID: @.***>

--

Dr. Jordi Faraudo Institut de Ciencia de Materials de Barcelona (ICMAB-CSIC), [ https://icmab.es/faraudo-gener-jordi-permanent-researchers | https://icmab.es/faraudo-gener-jordi-permanent-researchers ]

Associate Editor - Condensed Matter Physics @ "Heliyon" journal, [ http://www.heliyon.com/ | http://www.heliyon.com  ] Editorial Board member @ "Materials" journal, [ http://www.mdpi.com/journal/materials | http://www.mdpi.com/journal/materials ]

"It's nice to know the computer understands the problem, but I would like to understand it too." (Attributed to Eugene Wigner)

tonigi commented 1 year ago

I'll be happy to review.

majensen commented 1 year ago

Thanks so much @tonigi !

majensen commented 1 year ago

@editorialbot add tonigi as reviewer

editorialbot commented 1 year ago

I can't add that reviewer: tonigi is not a username

majensen commented 1 year ago

@editorialbot add @tonigi as reviewer

editorialbot commented 1 year ago

@tonigi added to the reviewers list!

majensen commented 1 year ago

@editorialbot remove @WangKehan573 as reviewer

editorialbot commented 1 year ago

@WangKehan573 removed from the reviewers list!

tonigi commented 1 year ago

Review checklist for @tonigi

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

tonigi commented 1 year ago

I've put my review at https://github.com/soft-matter-theory-at-icmab-csic/chitin_builder/issues/3#issuecomment-1825364955 . The software may be improved but it mostly works. There is no "API" (yet there should be) so i'm undecided on the "Functionality documentation"; i'm checking it for the time being.

jfaraudo commented 11 months ago

Hello @tonigi @amoeba @majensen , We have updated the code following the recommendations and also solved some issues for Windows and MAC users (in the case of MAC users for some versions it is necessary to do an additional action before running the code due to a VMD bug, as explained in the README). We are now going to update and shorten the paper follwong the recommendations by @amoeba . We will notify as soon as it is ready (we preferred to focus first on the code issues). Some specific comments/responses are also given below.

jfaraudo commented 11 months ago

@tonigi Concerning the speed of the code , we were not able to reproduce the slow speed reported in your example. We checked the speed in one of our desktop computers (Intel® Core™ i5-10400F CPU @ 2.90GHz × 12, NVIDIA Corporation TU116 [GeForce GTX 1650 SUPER]). It took 13 seconds to replicate a 11x11x11 alpha crystal (143,748 atoms). We think it is fairly fast, but of course the execution time is extremely device dependent and depends mostly on VMD code, such it is the one performing the calculations. We are not aware of any issue influencing the speed as the reviewer mentioned.

jfaraudo commented 11 months ago

Concerning the code structure, we have tried to improve it following @tonigi @amoeba comments. 1) We fixed the error with the the Documentation URL. 2) The replicate function was improved and moved some arguments to avoid conflicts between variables. 3) We have tried to reduce the warning messages from VMD. However, there are still some "duplicate atom key" warnings, which cannot be avoided, and should be ignored by the user. The reason is that they appear due to overlapping residues during the crystal building. These overlaps are then fixed with the patch command. The resetpsf command would not avoid these warnings since the “duplicate atom key” is just a result of how we replicate the crystal. In further updates we will look at a way to reduce these warning messages, but for the time being the simplest option is simply ignore it. 4) We fixed the requirement to navigate to the destination directory for every structure, every time starting from the system root. Now the default directory location is ~ . 5) We fixed the cancel button, it now aborts the generation of the crystal structure. 6) There was an issue with the "crystal.log" file, which it is often empty. We fixed this issue, the log file was not properly closed.

jfaraudo commented 11 months ago

The GUI and the main code are still all together in the same code, the script works invoking different procedures that can be called independently. We are thinking in reorganizing the code for future versions, but for now we are more focused on functionality. Actually, besides that the GUI and the main code are together in the same script, you can invoke the main replication function “replicate” by just loading the package and setting the working directory. To run from the VMD Tk console you need to run the following commands:

package require chitin set chitin::fname “/home/” chitin::replicate n1 n2 n3 crys1 per1

Where “\home\” should be replaced with your working directory. The arguments n1, n2 and n3 should be integers representing the number of replicas in x, y and z respectively. The argument crys1 should be the string “Alpha” or “Beta” depending on the chitin crystal structure to replicate. Finally the argument per1 should be the string “yes” or “no” depending if you want periodic bonds between the first and last residue.

We are aware of a bug that after using the Chitin Builder through the VMD command line, an error appears if afterwards you try to generate a crystal using the GUI. This bug can be worked around by just restarting VMD.

jfaraudo commented 11 months ago

We think this is all concerning the code. We will now update/shorten the paper asap.

majensen commented 10 months ago

Thanks for all this work @jfaraudo - would you like to comment on @tonigi 's concern (https://github.com/openjournals/joss-reviews/issues/5771#issuecomment-1825367156) about the package having an API?

jfaraudo commented 10 months ago

Thanks @majensen @tonigi . Sorry, I am not sure what exactly means an API in this context. But the commands from the tcl script can be called from outside the GUI, in particular from the Tk console, as we explained in the response (we will add further info on that in the revised paper). This means that the commands are also -in principle- callable from any other additional APP running in tcl in VMD. I think this should be enough, for the standard VMD simulation user/developer.

tonigi commented 10 months ago

By API I meant (loosely speaking) instructions to invoke the builder from TCL (i.e., from other scripts, not the GUI). It appears to be addressed in this comment https://github.com/openjournals/joss-reviews/issues/5771#issuecomment-1877382167 which possibly I guess deserves a brief section in the documentation.

jfaraudo commented 10 months ago

@tonigi Fully agree. We will add instructions to the documentation and an example on how to use it.

jfaraudo commented 10 months ago

@tonigi As suggested, we have added a brief comment in the README.md (with screenshots) and a brief section in the User Manual showing how to call the chitin builder from the VMD Tk console without using the GUI.

jfaraudo commented 10 months ago

@amoeba @majensen @tonigi the paper has been shortened and updated. We moved the material to the updated User Manual.

tonigi commented 10 months ago

@editorialbot generate pdf

editorialbot commented 10 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

tonigi commented 10 months ago

Looks good to me. A couple of typos: L.25 "on earth"; L.30 "po[s]sibility"; capitalise "GUI"; L.75 capitalise "GitHub". Maybe reference the PDB ID of the two allomorphs.

jfaraudo commented 10 months ago

@tonigi thanks, we will correct the typos. There are no PDB ID for these structures, these are from the references discussed in the methods section (now in the documentation).

jfaraudo commented 10 months ago

@majensen We have corrected the typos indicated by @tonigi (thanks for your careful reading!!) . Concerning the origin of the unit cells of the crystal structures , as mentioned before, they are not from PDB database so they do not have a PDB ID. They were .cif files provided in crystallographic publications and we converted them to pdb as described in the Method section of the user manual. We have added a line in the paper indicating this fact and pointing to the methods section in the User Manual for further technical details.

majensen commented 10 months ago

@jfaraudo thanks for this effort. @tonigi looks good to go from the checklist standpoint. @amoeba do you want to go over your checklist once more?

amoeba commented 10 months ago

Hey @majensen, I'm re-starting my review now and will report back shortly. At the very least, a new release of the software needs to be made and the JOSS submission needs to be updated.

amoeba commented 10 months ago

My review is nearly an accept, with only some minor revisions still needed:

Once those changes are made, we need:

jfaraudo commented 10 months ago

Thanks @amoeba for your comments, with fully agree with all your points.. 1) We accepted and merged your comments made to the README.md and paper.md files to the master branch. 2) We added community guidelines. A section has been added in the README.md which links to a file with a detailed description for third parties willing to contribute, report issues or seek support. 3) We have created a new software release (v1.1)

Concerning your last point ("An updated archive for JOSS") I am not sure what do you mean. Do you mean the paper.md file? (this has been updated form your PR). Or maybe is something that has to be done from the journal @majensen ? Or I am missing something? (sorry, we are new to this journal)

tonigi commented 10 months ago

@editorialbot generate pdf

editorialbot commented 10 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: