Skip to content

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

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

simongravelle
Copy link
Member

No description provided.

@simongravelle simongravelle self-assigned this Jun 30, 2025
@simongravelle simongravelle added this to the LiveCoMS-publication milestone Jun 30, 2025
@simongravelle
Copy link
Member Author

@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":

image

@simongravelle
Copy link
Member Author

@akohlmey @jrgissing What do you think of this reviewer's comment?

  1. Page 27 – The suffix of the parametrization file in case of “reaxCHOFe.inc” is misleading as this suffix was used, up to this point, to signify files with LAMMPS commands.

Would there be a better suffix for such files?

@simongravelle
Copy link
Member Author

simongravelle commented Jul 9, 2025

@akohlmey @jrgissing Another interesting comment concerning the use in tutorial 6 of :

pair_style hybrid/overlay vashishta lj/cut/tip4p/long OW HW OW−HW HW−OW−HW 0.1546 10

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?

@akohlmey
Copy link
Collaborator

akohlmey commented Jul 9, 2025

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

Yes, because we have no idea what those units would be. The text for thermo columns can be changed to anything with thermo_modify and only for a few selected fields, the units are easily known. But a user can change the y-axis legend manually. (same as the plot title).

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 thermo_modify as well.

@akohlmey
Copy link
Collaborator

akohlmey commented Jul 9, 2025

Would there be a better suffix for such files?

Following the convensions used in the LAMMPS potentials folder, the name for this file would become: ffield.reax.CHOFe or ffield.reax.C_H_O_Fe

@akohlmey
Copy link
Collaborator

akohlmey commented Jul 9, 2025

Apart from it being a bad combination, a reviewer suggest using hybrid instead of hybrid/overlay.

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.

@simongravelle
Copy link
Member Author

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.

Noted, will do.

@akohlmey
Copy link
Collaborator

@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)

@simongravelle
Copy link
Member Author

@jrgissing

Could you take care of the last remaining comment by Reviewer C?

I think it would be beneficial to have comment lines explaining the files that are used in the
REACTER protocol (see section 3.8.2) for a reader to understand the sections of each file, in case
there arent.

@akohlmey
Copy link
Collaborator

akohlmey commented Aug 5, 2025

@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.

@cecimarques
Copy link
Collaborator

cecimarques commented Aug 5, 2025

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
I think you can find more about it here: #68

@akohlmey
Copy link
Collaborator

akohlmey commented Aug 5, 2025

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?

If there was no merge conflict, everything will be fine.

@simongravelle
Copy link
Member Author

simongravelle commented Aug 6, 2025

@jrgissing, could you validate/correct the changes that were made here ?

Let us use \lmpcmd{fix bond/react} by adding the following
line to \flecmd{polymerize.lmp}:
\begin{lstlisting}
fix rxn all bond/react &
  stabilization yes statted_grp 0.03 &
  react R1 all 1 0 3.0 mol1 mol2 M-M.rxnmap &
  react R2 all 1 0 3.0 mol3 mol4 M-P.rxnmap &
  react R3 all 1 0 5.0 mol5 mol6 P-P.rxnmap
\end{lstlisting}
% CA: can someone confirm my changes added in this paragraph? I am unsure of them
% as I literally just figured it out to write them. (PS: I just realized that some
% of it may be redundant with the note, but I would still leave it as the reader
% will have the full information.
With the \lmpcmd{stabilization} keyword, the \lmpcmd{fix bond/react} command will
stabilize the atoms involved in the reaction using the \lmpcmd{nve/limit}
command with a maximum displacement of $0.03\,\mathrm{\AA{}}$.
{\color{blue}The \lmpcmd{fix nve/limit} command functions similar to
\lmpcmd{fix nve}, but restricts how far atoms can move in a single time step, even with
very large forces.}
By default, each reaction is stabilized for 60 time steps.  Each \lmpcmd{react} keyword
corresponds to a reaction, e.g.,~a transformation of \lmpcmd{mol1} into \lmpcmd{mol2}
based on the atom map \lmpcmd{M-M.rxnmap}.  Implementation details about each reaction,
such as the reaction distance cutoffs and the frequency with which to search for
reaction sites, are also specified in this command.

@jrgissing
Copy link
Collaborator

The snippet in your comment looks good. I would delete "based on the atom map \lmpcmd{M-M.rxnmap}", though.

@akohlmey
Copy link
Collaborator

akohlmey commented Aug 8, 2025

@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.

@simongravelle
Copy link
Member Author

@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.

@akohlmey
Copy link
Collaborator

akohlmey commented Aug 8, 2025

I think we’re all good.

That is great news. Thanks for all your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants