Skip to content

Conversation

@akohlmey
Copy link
Collaborator

This pull request contains revisions to the paper text and the tutorial files to make using them with LAMMPS-GUI smoother. Also, some LAMMPS-GUI test is revised and simplified, where possible, to reduce redundancy.

@akohlmey akohlmey self-assigned this Dec 30, 2024
@akohlmey
Copy link
Collaborator Author

@simongravelle I just noticed that the files for tutorial 7 include a Linux binary. I don't think that that is such a good idea (out of principle and for the tutorial). We have to assume that a large number of tutorial users will be using Windows and they usually don't have access to a compiler (or the knowledge to compile software for Windows).

How about I download and bundle the wham binary with LAMMPS-GUI?

If you want to keep distributing a binary, then it should be a fully static portable binary. I have the tools to create that, since I also use them for portable x86_64 binaries (very helpful to verify if bugs have been fixed).

@simongravelle
Copy link
Member

simongravelle commented Dec 31, 2024

I just noticed that the files for tutorial 7 include a Linux binary. I don't think that that is such a good idea (out of principle and for the tutorial). We have to assume that a large number of tutorial users will be using Windows and they usually don't have access to a compiler (or the knowledge to compile software for Windows).

Tutorial 7 primarily encourage the user to visit Grossfield Lab and follow the instruction from there. Initally the binary was given with the message 'you can also try your luck with the binary I compiled', but I understand that its not great.

@simongravelle
Copy link
Member

How about I download and bundle the wham binary with LAMMPS-GUI?

That would be awesome. Note that this code is not mine, but its documentation from the Grossfield lab says:

This code is available under the GPL and BSD li-
censes, as you prefer. The exception to this licensing is a set of routines from
Numerical Recipes, which are not mine to give away.
If you use this code as part of an original piece of research, I’d appreciate
a reference or acknowledgment. There’s no publication to reference, so please
use something like:
Grossfield, A, “WHAM: an implementation of the weighted histogram analy-
sis method”, http://membrane.urmc.rochester.edu/content/wham/, ver-
sion XXXX
For that matter, just letting me know what you’re using my code for would
be nice, although again I don’t insist upon it.

so with proper citation its probably fine.

@simongravelle
Copy link
Member

If you want to keep distributing a binary

Not particularly :)

@akohlmey
Copy link
Collaborator Author

akohlmey commented Jan 2, 2025

How about I download and bundle the wham binary with LAMMPS-GUI?

That would be awesome.

It took a little while to make it work across platforms, but I am now building new packages that include the "wham" and "wham-2d" executables. Will upload them soon.

I noticed that the wham executable uses a compile-time constant for k_B.

In the context of LAMMPS it would be more consistent to have an optional command-line flag for the units and then use the same units name to value mapping of LAMMPS (with the same values as LAMMPS uses). What do you think?

@simongravelle
Copy link
Member

What do you think?

Yes, that would be clearly better, but do you have an idea of the amount of work required? I don't mind having a look at it in the next weeks.

@akohlmey
Copy link
Collaborator Author

akohlmey commented Jan 2, 2025

Yes, that would be clearly better, but do you have an idea of the amount of work required? I don't mind having a look at it in the next weeks.

I don't expect a lot. There are only a few places where the define is used and that could be replaced by a global variable. There are several other global variables, so that won't be a big style change.
I have not looked at the command-line argument parser, though.

@akohlmey
Copy link
Collaborator Author

akohlmey commented Jan 2, 2025

do you have an idea of the amount of work required?

Please have a look at this patch. This will add CMake support for building and installing, but also adds a "units" keyword followed by the choice of units (same as in a LAMMPS input).
Same as the (optional) keyword for periodicity, it has to come at the beginning.

update-wham-2.0.11.patch.gz

@simongravelle
Copy link
Member

Please have a look at this patch. This will add CMake support for building and installing, but also adds a "units" keyword followed by the choice of units (same as in a LAMMPS input).
Same as the (optional) keyword for periodicity, it has to come at the beginning.

Thats really great, thank you for adding wham to the GUI in that way, it will make the tutorial so much cleaner to follow.

@akohlmey
Copy link
Collaborator Author

akohlmey commented Jan 4, 2025

Thats really great, thank you for adding wham to the GUI in that way, it will make the tutorial so much cleaner to follow.

FYI, I emailed my changes to upstream and they will be included in the next release. Apparently both, CMake support and setting units on the command line were planned features.

step during energy minimization (a) and as a function of time during
molecular dynamics in the NVT ensemble (b). b)~Kinetic energy
($k_\text{e}$) during energy minimization (c) and during molecular
dynamics (d).}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simongravelle this labeling is rather confusing. How about dropping "a)" and "b)" and just refer to the individual panels as a, b, c, and d?

The use of p_e and k_e is a bit unusual. E_pot, E_kin, or U, K would be more conventional. Unfortunately, the LAMMPS manual for both compute pe and compute ke does not provide a reference. The potential for the individual styles are usually referred to as U, though.

Is there perhaps a consistent convention in any of the text books the tutorial recommends?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of p_e and k_e is a bit unusual. E_pot, E_kin, or U, K would be more conventional. Unfortunately, the LAMMPS manual for both compute pe and compute ke does not provide a reference. The potential for the individual styles are usually referred to as U, though.

Good point, I created an issue, I will fix it #57

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #58

@akohlmey akohlmey marked this pull request as ready for review January 6, 2025 04:23
@akohlmey
Copy link
Collaborator Author

akohlmey commented Jan 6, 2025

@simongravelle I flagging this as ready even though I am not finished, but the changes, though mostly cosmetic, are growing too fast and merging multiple smaller PRs reduces the risk of and effort to resolve potential merge conflicts.

Copy link
Member

@simongravelle simongravelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

step during energy minimization (a) and as a function of time during
molecular dynamics in the NVT ensemble (b). b)~Kinetic energy
($k_\text{e}$) during energy minimization (c) and during molecular
dynamics (d).}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #58

@simongravelle simongravelle merged commit 02b102e into main Jan 6, 2025
@akohlmey akohlmey deleted the tutorial-file-revisions branch January 6, 2025 10:52
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.

3 participants