Removing dependence on pyiron_atomistics and depend on pyiron_lammps instead

[jan-janssen]

@ltalirz and @Atilaac I was able to fix the issues and the pull request is now ready for review. For some reason I can only specify one of you as reviewer but I guess it makes sense if both of you have a look at the changes to see if both the agent and the large atomistic simulations work fine with these changes. The major advantage is in reducing the number of dependencies by just using the LAMMPS parser rather than the full suite of atomistic tools in pyiron_atomistics.

[github-actions]

Coverage Report

File	Stmts	Miss	Cover	Missing
__init__.py	19	0	100%
io_utils.py	57	12	78%	106–111, 142, 171–172, 181, 185–186
mass.py	4	0	100%
neighbors.py	34	0	100%
shared.py	16	0	100%
structure.py	368	123	66%	89, 107–108, 126–127, 145–146, 149, 269, 271–272, 274–276, 279, 281–282, 306–307, 309–310, 326–328, 333–334, 340–341, 362–363, 370–371, 377, 381–383, 389–391, 394–395, 401–402, 411–412, 416–424, 508, 517, 519–520, 523, 525–533, 535–542, 553–554, 690–692, 701–702, 708, 715–716, 722–723, 728, 743, 761, 781, 794, 796–797, 800, 802, 804–807, 810–814, 988–989, 991–994, 997–1003, 1005–1007, 1010
analysis
__init__.py	0	0	100%
bond_angle_distribution.py	32	1	96%	79
cavities.py	19	0	100%
cte.py	30	9	70%	73, 84–85, 126–128, 131–132, 134
qn_network_connectivity.py	30	2	93%	103–104
radial_distribution_functions.py	68	18	73%	57–60, 62–63, 65–67, 69–71, 73–77, 79
rings.py	40	2	95%	88, 122
potentials
__init__.py	0	0	100%
bjp_potential.py	34	29	14%	45–46, 49, 56–58, 60, 68, 71–74, 77, 85–93, 95–99, 101, 103
pmmcs_potential.py	38	4	89%	66, 69, 72–73
potential.py	15	6	60%	32–37
shik_potential.py	84	73	13%	67–69, 71–73, 80–81, 83–86, 88–93, 95, 111–115, 117–118, 128–129, 131–132, 134–135, 138–143, 145–147, 150, 162–163, 165–166, 168–171, 174–181, 183, 185–188, 190, 192–196, 198–200, 202
workflows
__init__.py	0	0	100%
cte.py	207	57	72%	150–151, 216–220, 223–227, 230–234, 237–241, 244–248, 450, 454, 469–474, 477–484, 488–493, 523, 553–554, 769–773
elastic_mod.py	82	2	97%	316, 324
md.py	23	2	91%	171, 179
meltquench.py	31	5	83%	117–118, 192–194
meltquench_protocols.py	37	18	51%	129, 140, 151, 160, 170, 179, 186, 207, 218, 227, 237, 242, 252, 261, 271, 279, 334–335
shared.py	11	4	63%	27–29, 31
structural_analysis.py	244	10	95%	131–133, 135, 160–161, 179, 231, 247, 478
viscosity.py	149	42	71%	171, 179, 271, 317–318, 320–321, 323–324, 328–333, 339, 351, 353, 358–360, 362–363, 365–367, 369–370, 372–374, 376, 379–380, 384, 386, 411, 432–433, 456–457, 507
TOTAL	1672	419	74%

[ltalirz]

@jan-janssen The error is Failed: Meltquench task errored: Operands are not aligned. Do left, right = left.align(right, axis=1, copy=False) before operating.

I guess the error should happen somewhere here
https://github.com/glasagent/pyiron-glass/blob/77338cc5509b4b624d8696f3168ff72d8c824e30/pyiron_glass_api/src/pyiron_glass_api/worker.py#L55

If you can spot something potentially problematic, let me know.

Perhaps some adjustment in this file is required as well (?)

[ltalirz]

If one has to debug the calculation in more detail, I can also do that at some point but probably not before I'm back from parental leave

[jan-janssen]

@ltalirz I guess the more surprising part for me is that the regular tests work fine, while the agent tests failed even though the changes only affected the LAMMPS interface. Based on this I have the feeling the tests are somehow coupled which is something we should address. Is there anything you do different compared to the tests the @Atilaac does? Different parameters or something like this? I looked at it with @Atilaac but so far we were not able to identify any difference.

[ltalirz]

The code I linked is how the calculation is run in the API

I was also not aware of significant differences

If the error message does not point to something obvious I may need to have a deeper look later

[Atilaac]

@jan-janssen I believe I narrowed down the problem. When the code tries to call this function below, it triggers the error seen in the test because the simulations does not run without a potential.

potential_dataframe = get_potential_by_name( potential_name=potential, resource_path=resource_path )

https://github.com/pyiron/pyiron_lammps/blob/b789d91c01d6e54cb6afd656a3593c10ec915bb0/pyiron_lammps/compatibility/file.py#L90C5-L92C6

We provide the potential already as a dataframe and this should be changes to allow for that. I am not sure how to fix this. maybe by adding a check if the provided variable is already a dataframe then use it directly.

[jan-janssen]

@Atilaac Thanks a lot - I fixed this issue in a separate pull request https://github.com/pyiron/pyiron_lammps/pull/271/files#diff-d1807450b5bcd683d7d53e490562d7831904fa36ecfe67f8de4e0ba090b1d31b - pyiron/lammpsparser#271 - but that one is not merged yet. I am going to release a new version with the corresponding changes.

[jan-janssen]

"/home/runner/miniconda3/envs/test/lib/python3.13/site-packages/pyiron_lammps/compatibility/file.py", line 210, in _modify_input_dict
    key = l.split()[0]
          ~~~~~~~~~^^^
IndexError: list index out of range

[Atilaac]

When I try running the simulation using shik potential parameters the simulation fails. I was able to track where the issue is comming from. when I use this potential there will be a a line that add units metal after the definition of the simulation box which causes lammps to return an error (see the generated lammps script below. What is surprising to me is that this error does not happen for the other potentials and I have no idea why.

So far this is what I could comment on and those are the issues I was able to find.

units metal
dimension 3
boundary p p p
atom_style charge

read_data lammps.data
# S. Sundararaman et al.,
# for silica glass, J. Chem. Phys. 2018, 148, 19, https://doi.org/10.1063/1.5023707
# for alkali and alkaline-earth aluminosilicate glasses, J. Chem. Phys. 2019, 150, 15, https://doi.org/10.1063/1.5079663

# for borate glasses with mixed network formers, J. Chem. Phys. 2020, 152, 10, https://doi.org/10.1063/1.5142605

# for alkaline earth silicate and borate glasses, Shih et al. J. Non-Cryst. Sol. 2021, 565, 120853, https://doi.org/10.1016/j.jnoncrysol.2021.120853

units metal

### Group Definitions ###

group Al type 1
group Ca type 2
group O type 3
group Si type 4

### Charges ###

set type 1 charge 1.6334
set type 2 charge 1.4977
set type 3 charge -1.0379928571428572
set type 4 charge 1.7755

### SHIK Potential ###

pair_style hybrid/overlay coul/dsf 0.2 10.0 table spline 10000
pair_coeff * * coul/dsf
pair_coeff 1 1 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_Al_Al.tbl" SHIK_Buck_r24 10.0
pair_coeff 1 3 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_Al_O.tbl" SHIK_Buck_r24 10.0
pair_coeff 2 2 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_Ca_Ca.tbl" SHIK_Buck_r24 10.0
pair_coeff 2 3 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_Ca_O.tbl" SHIK_Buck_r24 10.0
pair_coeff 2 4 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_Ca_Si.tbl" SHIK_Buck_r24 10.0
pair_coeff 3 3 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_O_O.tbl" SHIK_Buck_r24 10.0
pair_coeff 3 4 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_O_Si.tbl" SHIK_Buck_r24 10.0
pair_coeff 4 4 table "/Users/achrafatila/Documents/Workflows/pyiron-glass/notebooks/table_Si_Si.tbl" SHIK_Buck_r24 10.0

`pair_modify shift yes

thermo_style custom step temp pe etotal pxx pxy pxz pyy pyz pzz vol
thermo_modify flush yes

thermo 100

fix langevin all langevin 5000 5000 0.05 48279

fix ensemble all nve/limit 0.5

run 10000
unfix langevin
unfix ensemble

variable dumptime equal 1
dump 1 all custom ${dumptime} dump.out id type xsu ysu zsu fx fy fz vx vy vz
dump_modify 1 sort id format line "%d %d %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g"
fix ensemble all nvt temp 5000.0 5000.0 0.1
variable thermotime equal 1
timestep 0.001
velocity all create 5000 12345 dist gaussian
thermo_style custom step temp pe etotal pxx pxy pxz pyy pyz pzz vol
thermo_modify flush yes
thermo ${thermotime}
run 100000

[jan-janssen]

@Atilaac Thanks for the feedback, I am going to work on this and get back to you. I mark the pull request as draft for now.

[ltalirz]

hey guys, what is the status here?
@Atilaac since the tests don't fail in the last commit, does this mean there is currently no test for the shik potential? would you be able to create a test for this in a separate PR?

[Atilaac]

Hi, in the current version of pyiron_atomistics this issue does not exist. A test for SHIK could be just running the notebook in the notebooks folder.

[ltalirz]

Ok, thanks. My suggestion would be to include at least a small test of the shik route in the integration tests (the test does not necessarily need to run a full simulation, it just needs to surface the error you witnessed).
That test would pass in the current main, but fail here.

[Atilaac]

I added a test that should fail for this PR and is working fine for the main branch.

[jan-janssen]

By migrating from pyiron_atomistics to pyiron_lammps we reduced the CI time for the Setup Mambaforge step from 1m 35s to 1m 10s and the download size from 975MB to 585MB while maintaining the same functionality. The 390MB of files are primarily related to DFT simulation so they are not required in this project.

[ltalirz]

Great!

[jan-janssen]

@Atilaac and @ltalirz I fixed the bugs in pyiron_lammps, so the unit tests are working fine. Can you take another look at this pull request? Once it is merged I am going to start working on the migration from pyiron_base to executorlib.

[Atilaac]

@jan-janssen I tested the PR locally and I found the following errors. I believe this is a bug in parsing or converting the pressure in pyiron_lammps. I will open a separate pull request in pyiron_lammps as I believe I fixed it locally.

[jan-janssen]

@jan-janssen I tested the PR locally and I found the following errors. I believe this is a bug in parsing or converting the pressure in pyiron_lammps. I will open a separate pull request in pyiron_lammps as I believe I fixed it locally.

@Atilaac Thanks a lot for the quick catch and the pull request to fix it. I patched the conda package. Can you test again with the latest version? You can install it using:

conda install pyiron_lammps=0.5.3=pyhdfbf58e_2

[Atilaac]

@jan-janssen This seems to work fine now. If you are done from your side I can merge this PR now. Let me know.

[jan-janssen]

@Atilaac I am done, but let’s give @ltalirz the chance to look at it as well and then we can merge it tomorrow.

[ltalirz]

thanks @jan-janssen , looks good to me!

[ltalirz]

just out of curiosity: is this the default way to build the commandline for running a code in pyiron (i.e. the user writes a python function to do it based on the server kwargs)?

[jan-janssen]

I guess the default way would be to have a shell script which has the module load commands and so on. In pyiron_atomistics we had an automated index over a predefined folder structure, this is helpful when you have multiple versions of executables but some times complicated to setup initially:
https://github.com/pyiron/pyiron-resources/blob/main/lammps/bin/run_lammps_2024.02.07.sh

Other tools like ASE use environment variables:
https://ase-lib.org/ase/calculators/lammpsrun.html

At the moment the pyiron_lammps interface just accepts a command which is then executed with a subprocess as I see pyiron_lammps primarily as a writer for the input files and a parser for the output files. To be backwards compatible with the pyiron_atomistics interface we added the option for the user to specify the command.