-
Notifications
You must be signed in to change notification settings - Fork 1
Applied comments from Livecoms Reviewer #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@akohlmey Something that came to my mind while applying the reviewer's comments, would it be difficult to have the units appear on the axis of the LAMMPS-GUI Charts when printing thermo keywords of LAMMPS, such as "Density": |
@akohlmey @jrgissing What do you think of this reviewer's comment?
Would there be a better suffix for such files? |
@akohlmey @jrgissing Another interesting comment concerning the use in tutorial 6 of :
Apart from it being a bad combination, a reviewer suggest using hybrid instead of hybrid/overlay. I am actually thinking that it would be best to completely remove the "hybrid/overlay vashishta" from this part, and then only have one single potential lj/cut/tip4p/long, leaving the silica rigid as is not uncommon for GCMC simulations. It would be cleaner, but then there would not be a single use of hybrid pair style in the entire tutorials. What do you think is best? |
Yes, because we have no idea what those units would be. The text for thermo columns can be changed to anything with What could be useful here is a) display the LAMMPS units settings and b) if the thermo output is normalized (like for units lj by default) since that can be changed with |
Following the convensions used in the LAMMPS potentials folder, the name for this file would become: |
The comment is correct. Pair style hybrid/overlay is not needed. Unfortunately, this is a convention that was started by Andrew Jewett's moltemplate and is propagating. Pair style hybrid/overlay gives you the most flexibility when combining multiple force fields (but also the largest margin for error due to lack of mixing and potential double counting of interactions). Yes, vashishta can only be used reliably for bulk systems. I think making the silica immobile (don't use the term rigid here, rigid is still mobile but as a whole) is the best solution without having to have major edits to the paper. I think the issue of hybrid potentials is worth a more general treatment, like in a Howto document in LAMMPS. That would allow to discuss examples rather than explain features like in the parts documenting the commands. That would allow, for example, to use some drastic words showing how bad using hybrid with EAM is. |
Noted, will do. |
@simongravelle We should require LAMMPS-GUI version 1.7 which is included in LAMMPS version 29Aug2024-update4 (not yet released) and LAMMPS version 30Jul2025 (not yet released and release date subject to change if we don't get all pending pull requests not flagged for after the stable release merged) |
Could you take care of the last remaining comment by Reviewer C?
|
with the \hspace{0pt} a following punctuation will not "stick" but may wrap to the next line.
also some small corrections and reformulations are added.
@simongravelle I just completed a pass with a few text adjustments and especially updates to the figure placement and general layout. There should be at most one figure per column and the figure should be either on the same page of the first mention or the page before or after. Also, they should be now shown in order. I also tried to avoid larger empty blocks. With the many "note" environments (which cannot be split) this was quite difficult and not 100% successful. If there are other tasks you have for me, please send me an email with the details. |
Update lammps-tutorials.tex
Axel, you had editted the .tex before I committed my changes and I had an interesting problem that people in my office helped me solve (involves merging pull requests when there is no conflict) since I was apparently born in the 1800s and cannot understand github very well. Can you please, just for my peace of mind, verify if everything is okay with one of the changes you proposed or if I have destroyed all your work? heheheheheh |
If there was no merge conflict, everything will be fine. |
@jrgissing, could you validate/correct the changes that were made here ?
|
The snippet in your comment looks good. I would delete "based on the atom map \lmpcmd{M-M.rxnmap}", though. |
@simongravelle How are we doing with the review updates? Is there anything you want me to do? It looks like I will have some time today. |
@akohlmey I think we’re all good. I believe @cecimarques wanted to do some additional testing, and after that I’ll merge the PR. I will then send the "black and blue pdf" and the answers to Reviewers to the Livecoms journal. Finally, I will remove the blue color and push the pdf to GitHub, as well as uploading a v2 on arxiv. It will be done before the start of the Workshop. I’ll also need to make sure that all changes from the .tex file are synchronized with the changes on the website. This will be a bit painful, but it’s my burden. |
That is great news. Thanks for all your efforts. |
No description provided.