Skip to content

Started applying 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 17 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)

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.

2 participants