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

Issue reading castep's lattice_abc #107

Closed
jryates opened this issue Jul 25, 2020 · 5 comments · Fixed by #247
Closed

Issue reading castep's lattice_abc #107

jryates opened this issue Jul 25, 2020 · 5 comments · Fixed by #247

Comments

@jryates
Copy link

jryates commented Jul 25, 2020

Line 81 of io/castep.py is
lengths_and_angles = lattice_abc[1:]
This causes an error, I think the correct line is
lengths_and_angles = lattice_abc

This is the section of code which reads a lattice_abc block - specifically the case in which we haven't passed a unit line (and hence we don't need to strip out the 1st row)

I didn't make a pull request because the run then gives a warning:
DeprecationWarning: from_lengths_and_angles is deprecated Use Lattice.from_parameters instead. This will be removed in v2020.*
So I thought it might be better to fix the two things at the same time. I'm still getting the hang of python, so I'll defer to @ajjackson

@ajjackson
Copy link
Member

Thanks for reporting, I'll get to this when I can!

@jryates
Copy link
Author

jryates commented Feb 18, 2022

Coming back to this 18months later, I see that this fix is no longer enough as
lattice = Lattice.from_lengths_and_angles(*lengths_and_angles)
has now been removed.

Replacing this call with

 lattice = Lattice.from_parameters(lengths_and_angles[0][0],
     lengths_and_angles[0][1],
     lengths_and_angles[0][2],
     lengths_and_angles[1][0],
     lengths_and_angles[1][1],
     lengths_and_angles[1][2])

seems to work. I could make a pull request for this - however, I think there is probably a more elegant python fix, so would prefer to defer to a more competent python coder!

@ajjackson
Copy link
Member

Thanks for the investigation and reminder Jonathan, I'll take a look.

@ajjackson
Copy link
Member

@utf I've had a look at this and run into a problem: Lattice.from_parameters does not generate the cell in lower-triangular form as CASTEP does. This would give an inconsistent structure if the atom positions are given in absolute coordinates.

Do you know of any functionality in Pymatgen to standardise the cell to lower-triangular? It's easy in ASE...

@ajjackson
Copy link
Member

@jryates Looking at subroutine cell_abc_to_cart_lattice(a,b,c,alpha,beta,gamma,mycell) in the CASTEP source, it is somewhat complex and uses the spacegroup. Perhaps we should refuse to read .cell files that use lattice_abc with positions_abs, rather than risk getting it wrong.

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 a pull request may close this issue.

2 participants