aiida-vasp / parsevasp

A general parser for VASP
MIT License
13 stars 13 forks source link

Add option to allow Cartesian printing of positions in `POSCAR` #51

Closed JPchico closed 3 years ago

JPchico commented 3 years ago

Right now even if one sets direct=True in the Site class in the the POSCAR parser, internally it will always be transformed to direct coordinates.

This can be detrimental since there can be situations in which printing the coordinates in Cartesian coordinates is necessary.

JPchico commented 3 years ago

@espenfl I was thinking of making a PR about this, and I think that the best way is to have a property of the Poscar class called direct. of this way when the file is read if direct is True then everything continues as expected, but if direct is False then a conversion to cartesian coordinates is performed. And the Poscar is written in cartesian coordinates. What do you think?

espenfl commented 3 years ago

@JPchico This choice was done to make sure the POSCAR is always in direct coordinates. Working with cartesian will always work, but you have to do this on the AiiDA side, e.g. using the routines for the StructureData etc. Then load them to VASP using the parser in direct coordinates. Is there a use case where this would not fit?

Or maybe you mean if you use this parser as a stand alone or in other projects? For that case, I agree it would be nice. So yes, let us add it the possibility to parsevasp.

JPchico commented 3 years ago

@espenfl I'm not sure I follow in the AiiDA part, what I notice is that I tried to use a structure that is in cartesian coordinates and then is always written as direct coordinates in the POSCAR, this will not work for example if you are calculating stacking fault energies using the tilting method, as the cell vectors are changed but the atomic positions are kept constant in cartesian coordinates, if you try to do this with direct coordinates, as the atomic positions are relative this will cause in one missing the physics that you are tying to capture.

espenfl commented 3 years ago

@JPchico Yes, that was a quick one, and the reason why edited and added the second line :) This parser should be decoupled from AiiDA for sure.

As a side note, for this particular use case, do you use a custom script of could you use AiiDA? If that is the case, you regenerate the direct positions before launching the new calculation with the tilted new cell, so it should work fine. But if you for instance call some other script that only modifies the cell and not the positions it would not.

espenfl commented 3 years ago

@JPchico would you be happy with me editing your PR with my suggestions and adding a test as well?

JPchico commented 3 years ago

@espenfl Sure thing, I was planning to add what you suggested but I was very busy the last couple of weeks fixing our cluster that has crashed and I'm administrating.

I use AiiDA, what I have is a very simplistic workchain, in which I pass the initial structure and internally I generate the tilted cells. I need to check what you suggest of regenerating the direct coordinates, I thought I tried that and it failed but perhaps I was doing something wrong, will check it anyhow.

espenfl commented 3 years ago

@JPchico Yes, okey. I think maybe it would make sense to add some routine to the StructureData or some other utils place for these things when we need it as more people can then use it. Then we also know people use the same code snippet. If we have it on the parser side, it is very coupled to VASP. But its functionality is pretty general. I believe it should work as we can choose the cartesian or direct as we see fit. But of course any update probably need to trigger a regeneration of the entries, in particular the positions to be representable. Happy to have a look at that part of the script to see if we can find the issue.

espenfl commented 3 years ago

Closed by #52.