Skip to content

Create cell class and remove ase#247

Draft
jimnel wants to merge 8 commits intomainfrom
rm_ase_dependence
Draft

Create cell class and remove ase#247
jimnel wants to merge 8 commits intomainfrom
rm_ase_dependence

Conversation

@jimnel
Copy link
Contributor

@jimnel jimnel commented Feb 23, 2026

This resolves #12

  • Creates a new cell class that replaces ase.Cell.
  • With this ase is removed from qse.

@jimnel jimnel requested a review from a team February 24, 2026 09:16
@jimnel jimnel marked this pull request as ready for review February 24, 2026 09:16
Copy link
Contributor

@sherryblair sherryblair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good overall, I added one suggestion

numpy.ndarray
The reciprocal cell, calculated as 2π * inverse of the
transposed cell matrix.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add an error message incase the matrix has no inverse?

if self.rank < 3:
raise ValueError("Reciprocal lattice undefined for rank < 3.")

sherryblair
sherryblair previously approved these changes Mar 4, 2026
@rajarshitiwari
Copy link
Collaborator

The Cell class I feel needs a bit more work before we merge. @jimnel Please look at the methods that ase.Cell offeres. While there are several methods that we may not need, there are some that I think we should have have.

Also, while we are changing it. We need to implement it in a way that removes one drawback that original cell object had, and that was forcing 3D structure. Let's give it some thought to see if we can make it a bit flexible for 1D, 2D, 3D. If it turns out hectic, we back out and keep 3D.

A point to note: the cell.rank attribute in original ase.Cell doesn't really compute the rank of the 3D matrix, but rather computes the number of non-zero lattice vectors, and it also uses it to get reciprocal in reduced dimension.

"""
if self.rank() < 3:
raise ValueError("Reciprocal lattice undefined for rank < 3.")
return 2 * np.pi * np.linalg.inv(self.lattice_vectors.T)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be some kind of mask to do reciprocal in reduced dimension. See the original ase.Cell class for reference.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimnel Check this snippet from original ase.Cell for reciprocal

def reciprocal(self) -> 'Cell':
        """Get reciprocal lattice as a Cell object.

        The reciprocal cell is defined such that

            cell.reciprocal() @ cell.T == np.diag(cell.mask())

        within machine precision.

        Does not include factor of 2 pi."""
        icell = Cell(np.linalg.pinv(self).transpose())
        icell[~self.mask()] = 0.0  # type: ignore[index]
        return icell

@jimnel
Copy link
Contributor Author

jimnel commented Mar 5, 2026

@rajarshitiwari
We can discuss this on Friday. I've a few ideas.

@jimnel jimnel marked this pull request as draft March 6, 2026 13:43
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 this pull request may close these issues.

Remove ase dependence

3 participants