-
Notifications
You must be signed in to change notification settings - Fork 1
Reax ff #122
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: master
Are you sure you want to change the base?
Reax ff #122
Conversation
| # set environment for LAMMPS and msi2lmp executables | ||
| # to find potential and force field files | ||
| if ( "$?LAMMPS_POTENTIALS" == 0 ) setenv LAMMPS_POTENTIALS /home/brandon/.local/share/lammps/potentials | ||
| if ( "$?MSI2LMP_LIBRARY" == 0 ) setenv MSI2LMP_LIBRARY /home/brandon/.local/share/lammps/frc_files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal files such as these can be added to .git/info/exclude
| volume = 38, | ||
| pages = {245--259} | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files like this which may be accidentally created could be added to .gitignore (a line with log.cite would do)
| "atom_modify map array", // store all positions in an array | ||
| "atom_modify sort 0 0.0", // don't reorder atoms during simulation | ||
| ])?; | ||
| if &(pair_style.to_string())[..17] == "pair_style reax/c"{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to
if pair_style.to_string().starts_with("pair_style reax/c") {More importantly, however, rather than checking these things here, it would be preferrable to make it possible to handle these changes by adding methods to abstract over this to the rsp2_lammps_wrap::Potential trait. For now though, until we are certain what methods are needed, I am okay with this hack.
| pub fn a() -> f64 { 1.0 } | ||
| pub fn vacuum_sep() -> f64 { 10.0 } | ||
| pub fn layer_sep() -> Either<f64, Vec<f64>> { Either::A(1.0) } | ||
| pub fn layer_sep() -> Either<f64, Vec<f64>> { Either::A(1.5) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try and see if adding layer-sep: 1.5 to your layers.yaml files works, rather than changing the default.
src/io/structure/layers_yaml.rs
Outdated
| // Number of unique images to generate along each layer lattice vector | ||
| #[serde(default = "defaults::layer::repeat")] | ||
| pub repeat: [u32; 2], | ||
| pub site_atoms: Vec<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make this Vec<Element> then you shouldn't need the conversion loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think "atoms" is vague. For a public interface like this I think the preferred name would be site-symbols. elements would also work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try Vec<Element> here? What happens? I know I also suggested this before submitting the PR.
This would be what I intended to work, so if this produces a missing trait impl error then that's a bug in Element.
| ) => bail!("cannot scale layer separations when layers have not been determined"), | ||
|
|
||
| ( | ||
| &cfg::Scalable::UniformLayerSep { ref mask, .. } | &cfg::Scalable::LayerSeps { ref mask, .. }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm I wonder why these changes are showing up here, did I not merge them into master?
src/tasks/filetypes/eigensols.rs
Outdated
| // Conversion factor phonopy uses to scale the eigenvalues to THz angular momentum. | ||
| // = sqrt(eV/amu)/angstrom/(2*pi)/THz | ||
| const SQRT_EIGENVALUE_TO_THZ: f64 = 15.6333043006705; | ||
| const SQRT_EIGENVALUE_TO_THZ: f64 = 15.6333043006705 * 0.208; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes! We'll definitely need to figure out a better way to adjust this.
(also, you should document where the 0.208 comes from)
ExpHP
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more thing
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'll have to try and recall what the deal with this was, but I'd like to avoid just deleting this...
No description provided.