Skip to content

Commit

Permalink
Changes to lattice point group and prim factor group operation findin…
Browse files Browse the repository at this point in the history 
…g to result in operations with elements exactly equal to zero in more cases where they are very very close to zero.
  • Loading branch information
@bpuchala
bpuchala committed Aug 6, 2024
1 parent 4901ca2 commit 28f1140
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 7 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Added `IntegralSiteCoordinateRep.copy`, `IntegralSiteCoordinateRep.__copy__`, `IntegralSiteCoordinateRep.__deepcopy__`, and `IntegralSiteCoordinateRep.__repr__`
- Added usage documentation for Structure manipulation and input / output


### Changed

- Changed `IntegralSiteCoordinate.__str__` to `IntegralSiteCoordinate.__repr__` and changed the output from "b, i j k" to "[b, i, j, k]"

- Changed `xtal::make_point_group` to remove the `symmetrized_with_fractional` step.
- Changed construction of factor group translations to set components very close to zero (absolute value less than tol * 1e-5) to exactly zero.

### Fixed

Expand Down
2 changes: 1 addition & 1 deletion include/casm/crystallography/SymType.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ typedef bool SymOpTimeReversalType;

/// Within the scope of crystallography, this struct will serve as the symmetry
/// object, which holds a transformation matrix, translation vector, and time
/// reversal boolean, whithout any other overhead.
/// reversal boolean, without any other overhead.
struct SymOp {
SymOp(const SymOpMatrixType &mat, const SymOpTranslationType &translation,
SymOpTimeReversalType time_reversal)
Expand Down
12 changes: 12 additions & 0 deletions src/casm/crystallography/BasicStructureTools.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,18 @@ xtal::SymOpVector make_factor_group_from_point_group(
// correct this.
translation -= drift;

// If components of the translation are very close to zero,
// just set them to zero
{
Eigen::VectorXd translation_cart = translation.const_cart();
for (int i = 0; i < 3; ++i) {
if (CASM::almost_zero(translation_cart[i], tol * 1e-5)) {
translation_cart[i] = 0.0;
}
}
translation.cart() = translation_cart;
}

// Now that the translation has been adjusted, create the symmetry
// operation and add it if we don't have an equivalent one already
xtal::SymOp translation_operation =
Expand Down
6 changes: 2 additions & 4 deletions src/casm/crystallography/SymTools.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,9 @@ std::vector<SymOp> make_point_group(Lattice const &_lat, double tol) {
std::vector<SymOp> result;
result.reserve(frac_point_group.size());
Eigen::Matrix3d t_cart, t_diff;
Lattice symlat = symmetrized_with_fractional(tlat_reduced, frac_point_group);

This comment has been minimized.

Sign in to view

Copy link
@jcthomas

jcthomas Aug 9, 2024

I'm curious why this was removed? Using the symmetrized lattice here guarantees that the Cartesian symop will be an isometry and that the Cartesian symops will form a closed group.

This comment has been minimized.

Sign in to view

Copy link
@bpuchala

bpuchala Aug 10, 2024

Author Collaborator

The symmetrized lattice step results in matrices that have very small non-zero components when they should really be exactly zero for common lattices given in reduced form and aligned with the Cartesian axes. This makes it harder to read and quickly understand symop matrices as well as the lattices & superlattices that have been multiplied by symop matrices to put them into canonical form. For some tutorials I was trying to figure out where the small numbers that made the output more messy were coming from and traced it to this step (and the drift calculation step for factor group translations). Rather than use rounded numbers in many places where symop or lattice vectors are output I thought it was better to simplify this method and get the exact result in the common cases and deal with symmetrization explicitly when needed.

Please correct me if I'm missing something, but I understand the previous step as ensuring group closure and this step as making the mapping of lattice points under the Cartesian op matrices approximately equal to a tighter tolerance given a set of operations that have already been determined.

In that case, I think it is better to keep the exact results for the most common cases and for the marginal cases where equivalences are sensitive to the tolerance it will be better to detect that and handle the symmetrization in a way that informs the user what is happening. This would be like the check for tolerance sensitive prim in casm init where we suggest symmetrization before proceeding.

This comment has been minimized.

Sign in to view

Copy link
@jcthomas

jcthomas Aug 12, 2024

I see, that does make sense, and I'm curious what types of lattices you see this problem for (trigonal or hexagonal possibly?).

So, just to clarify my earlier point, the point group is first constructed in fractional coordinates, resulting in integer matrices, and its closer is found, which should results in a multiplication table corresponding to one of the 32 crystallographic point groups. Then, it is converted to its Cartesian representation. However, this conversion implicitly assumes that the lattice vectors are exactly invariant to the identified point group, which may not be the case (due to crystallographic tolerance, and the additional group closure step. The symmetrized lattice is guaranteed to satisfy this assumption (to within ~machine tolerance), but the original input lattice is not.

The end result is that, if the symmetrized lattice is not used for the conversion to the Cartesian representation, there will be small inaccuracies in the resulting matrices (e.g., rotation by 120.005° instead of 120°). I think what I said in my original comment about the matrices not being isometries isn't correct -- rather, the issue is that the action of the Cartesian representation of the point group doesn't precisely satisfy the multiplication table.

This becomes an issue later when CASM constructs symmetry-invariant DoF monomials. At that point, it doesn't really matter that the SymOps were constructed using some crystallographic tolerance -- the basis functions should be definitively invariant to the imposed point group.

I'm not sure what the best solution is here. I agree that symmetrizing the lattice can slightly shift its orientation, even though there are some pains taken to avoid doing so -- maybe that can be improved. But, I do think that removing the symmetrization step can result in unpredictable and hard-to-diagnose knock-on effects later on (e.g., unexpected symmetry-breaking behavior for simulations involving continuous DoFs).

This comment has been minimized.

Sign in to view

Copy link
@bpuchala

bpuchala Aug 12, 2024

Author Collaborator

That makes sense and it is good to think through where we should add checks that the result of using the symmetry operations has the expected symmetry / number of equivalents / etc. Thanks for pointing out this potential consequence.

I'm curious what types of lattices you see this problem for

The specific case I was looking at originally was FCC lattices. The symmetrization step does cause more small errors with BCC and hexagonal lattices also.

I tested more cases and skipping the symmetrization step still doesn't solve the issue for a lot of cases - more than I anticipated. Quite a lot of lattices with rational numbers in both lat_column_mat and inv_lat_column_mat still get small non-zero elements in the resulting Cartesian op matrices depending on the lattice vector length due to the floating point operations. This change just happens to fix several cases I tested originally. To fully address the issue we'd need a tolerance / rounding approach here or at print statements / output.

I'm adding __repr__ methods for the Python classes to make print(x) work, where possible by using existing methods to write the object as a JSON-formatted string. Overall it seems easier, and less prone to error or misunderstanding, to address the issue by using tolerances / rounding at the point symop or superlattice are constructed instead of at all the places where we do output - but I don't think it's entirely clear cut.

I also still find it preferable to skip a symmetrization step here because of the small errors it does introduce, because I think it is harder to understand, and because I don't think it's really okay to avoid some of those checks later anyway.
for (Eigen::Matrix3i const &frac : frac_point_group) {
t_cart = symlat.lat_column_mat() * frac.cast<double>() *
symlat.inv_lat_column_mat();
t_diff = t_cart * _lat.lat_column_mat() - _lat.lat_column_mat();
t_cart = tlat_reduced.lat_column_mat() * frac.cast<double>() *
tlat_reduced.inv_lat_column_mat();
result.push_back(SymOp::point_operation(t_cart));
}
return result;
Expand Down

0 comments on commit 28f1140

Please sign in to comment.