Skip to content
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

[ERROR] LAMMPS Format Option Segfaults #35

Closed
ejmeitz opened this issue Jan 7, 2024 · 10 comments · Fixed by #36
Closed

[ERROR] LAMMPS Format Option Segfaults #35

ejmeitz opened this issue Jan 7, 2024 · 10 comments · Fixed by #36
Labels

Comments

@ejmeitz
Copy link

ejmeitz commented Jan 7, 2024

When using the option output_format on both the generate_structure and canonical_configuration (and I expect any binary that has this option) I get an error when using the LAMMPS format.

The command I ran was from the MgO example, but I added the -of 3 option to the end:
canonical_configuration -n 4 -t 300 -mf 20 --quantum -of 3

At line 571 of file type_crystalstructure_io.f90 (unit = 100, file = 'lammps_conf0001')
Fortran runtime error: Expected INTEGER for item 4 in formatted transfer, got REAL
(i10,e20.8,a)
 ^
@mjv500
Copy link
Contributor

mjv500 commented Jan 7, 2024

Hi - please pull and check (and confirm) if the format fix is ok.

@flokno
Copy link
Contributor

flokno commented Jan 8, 2024

@mjv500 maybe a good motivation to include #10 as well (preferably via a PR that closes both issues, but we can do that manually of course)?

@mjv500
Copy link
Contributor

mjv500 commented Jan 8, 2024 via email

@ejmeitz
Copy link
Author

ejmeitz commented Jan 8, 2024

The error is now on the next item of the format string:

At line 571 of file type_crystalstructure_io.f90 (unit = 100, file = 'lammps_conf0001')
Fortran runtime error: Expected INTEGER for item 5 in formatted transfer, got CHARACTER
(i10,e20.8,2a)
^

flokno added a commit to flokno/tdep that referenced this issue Jan 9, 2024
@flokno
Copy link
Contributor

flokno commented Jan 9, 2024

Ok @ejmeitz this should fix it: #36

Please confirm on your end and we're good to go

@ejmeitz
Copy link
Author

ejmeitz commented Jan 9, 2024

Command runs well now, I believe you need an new (empty) line after the "Masses" heading for it to get parsed right though. With that space it gets parsed correctly. (quite the annoying format in my opinion).

The command also prints Have not fixed velocity output for LAMMMPS yet to the terminal. Not sure if you meant to leave that there.

Here's an example file that LAMMPS generated internally.
initial_structure_LJ.txt

@flokno
Copy link
Contributor

flokno commented Jan 10, 2024

@ejmeitz ok thanks for checking. LAMMPS seems to be a little problematic.

We discussed the issue and tend towards removing native LAMMPS support as we don't use (and maintain) it ourselves. Converting (default) VASP POSCARs to Lammps is as easy as e.g.

ase convert contcar_conf0001 -i vasp lammps.in -o lammps-data (--write-args masses=True)

Would that be fine?

@ejmeitz
Copy link
Author

ejmeitz commented Jan 10, 2024

Fine by me, I dont actually use the LAMMPS format was just testing stuff for the JOSS paper and this was the thing that broke. I guess I'd say just remove it as an option rather than leave it half working maybe?

That format is problematic in many ways, I understand why this would be faster.

@flokno
Copy link
Contributor

flokno commented Jan 10, 2024

@ejmeitz ok I removed lammps altogether now. I'll update the docs once we all agree on this.

In case somebody would try to use the lammps interface, we now exit with

 ERROR
 exit code 8: Feature removed

 Native LAMMPS IO was removed, please use external converters.

I also removed the holding stuff from build_things.sh

flokno added a commit that referenced this issue Jan 10, 2024
* io | fix lammps output

- #35

* io | fix Siesta output

- #10

* io | remove LAMMPS writer

- remove from crystalstructure_io
- remove from programs
- remove from docs

* build_things | remove holding from public version
@flokno
Copy link
Contributor

flokno commented Jan 10, 2024

Public website is also updated

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

Successfully merging a pull request may close this issue.

3 participants