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

Breaking: fix SubstrateAnalyzer film + substrate vectors not using original crystal coordinates #3572

Merged
merged 7 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 38 additions & 32 deletions pymatgen/analysis/interfaces/substrate_analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
from pymatgen.core.surface import SlabGenerator, get_symmetrically_distinct_miller_indices

if TYPE_CHECKING:
from numpy.typing import ArrayLike

from pymatgen.core import Structure


Expand Down Expand Up @@ -95,50 +97,59 @@ def __init__(self, film_max_miller=1, substrate_max_miller=1, **kwargs):
Initializes the substrate analyzer

Args:
zslgen(ZSLGenerator): Defaults to a ZSLGenerator with standard
zslgen (ZSLGenerator): Defaults to a ZSLGenerator with standard
tolerances, but can be fed one with custom tolerances
film_max_miller(int): maximum miller index to generate for film
film_max_miller (int): maximum miller index to generate for film
surfaces
substrate_max_miller(int): maximum miller index to generate for
substrate_max_miller (int): maximum miller index to generate for
substrate surfaces.
"""
self.film_max_miller = film_max_miller
self.substrate_max_miller = substrate_max_miller
self.kwargs = kwargs
super().__init__(**kwargs)

def generate_surface_vectors(self, film_millers, substrate_millers):
def generate_surface_vectors(
self, film: Structure, substrate: Structure, film_millers: ArrayLike, substrate_millers: ArrayLike
):
"""
Generates the film/substrate slab combinations for a set of given
miller indices.

Args:
film_millers(array): all miller indices to generate slabs for
film (Structure): film structure
substrate (Structure): substrate structure
film_millers (array): all miller indices to generate slabs for
film
substrate_millers(array): all miller indices to generate slabs
substrate_millers (array): all miller indices to generate slabs
for substrate
"""
vector_sets = []

for f in film_millers:
film_slab = SlabGenerator(self.film, f, 20, 15, primitive=False).get_slab()
film_vectors = reduce_vectors(film_slab.lattice.matrix[0], film_slab.lattice.matrix[1])
for f_miller in film_millers:
film_slab = SlabGenerator(film, f_miller, 20, 15, primitive=False).get_slab()
film_vectors = reduce_vectors(
film_slab.oriented_unit_cell.lattice.matrix[0], film_slab.oriented_unit_cell.lattice.matrix[1]
Copy link
Member

Choose a reason for hiding this comment

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

you can assign this to a variable to not have to repeat it 4 times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for replying late for I was leaving back home recently. I am new with Github. Could you please tell me how can I change the file directly in this 'issue' page? or should I make another pull request?

Copy link
Contributor

@hongyi-zhao hongyi-zhao Feb 7, 2024

Choose a reason for hiding this comment

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

@janosh
you can assign this to a variable to not have to repeat it 4 times

I have tried several times to interpret your above description, but it seems that in the end, I still do not quite understand your intention.

Copy link
Member

Choose a reason for hiding this comment

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

@hongyi-zhao i just meant intermediate variables with shorter names can make the code more readable here. but the main thing missing is a test.

@jinlhr542 how did you create your PR? usually people clone the repo, make local edits, push them to a fork and make a PR. if you add commits to your fork, they'll show up in this PR. you can also use GH web and edit your PR online by simply pressing . while on this page

Copy link
Contributor

@hongyi-zhao hongyi-zhao Feb 8, 2024

Choose a reason for hiding this comment

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

@janosh

i just meant intermediate variables with shorter names can make the code more readable here.

What about the following modified version?

for f in film_millers:
    film_slab = SlabGenerator(self.film, f, 20, 15, primitive=False).get_slab()
    oriented_lattice_f = film_slab.oriented_unit_cell.lattice  # For readability
    fv1, fv2 = oriented_lattice_f.matrix[0], oriented_lattice_f.matrix[1]  # Shorter names for vectors
    film_vectors = reduce_vectors(fv1, fv2)

    for s in substrate_millers:
        substrate_slab = SlabGenerator(self.substrate, s, 20, 15, primitive=False).get_slab()
        oriented_lattice_s = substrate_slab.oriented_unit_cell.lattice  # Similar change for substrate
        sv1, sv2 = oriented_lattice_s.matrix[0], oriented_lattice_s.matrix[1]  # Shorter names for vectors
        substrate_vectors = reduce_vectors(sv1, sv2)

but the main thing missing is a test.

How about the following as a starting point?

import unittest
from pymatgen import Lattice, Structure
from pymatgen.analysis.interfaces.substrate_analyzer import SubstrateAnalyzer

class TestSubstrateAnalyzer(unittest.TestCase):

    def setUp(self):
        # Create mock film and substrate structures for testing
        lattice = Lattice.cubic(3.5)
        species = ["Cu"]
        coords = [[0, 0, 0]]
        self.mock_film = Structure(lattice, species, coords)
        self.mock_substrate = Structure(lattice, species, coords)

        # Initialize the SubstrateAnalyzer with mock structures
        self.analyzer = SubstrateAnalyzer()

    def test_generate_surface_vectors(self):
        # Define Miller indices for film and substrate
        film_miller_indices = [(1, 0, 0)]
        substrate_miller_indices = [(1, 1, 1)]

        # Use the method under test to generate surface vectors
        vector_sets = self.analyzer.generate_surface_vectors(film_miller_indices, substrate_miller_indices)

        # Assert that vector sets are generated
        self.assertTrue(vector_sets, "Expected non-empty list of vector sets.")

        for film_vectors, substrate_vectors, f, s in vector_sets:
            # Assert that the vectors have length 2 (indicating reduced vectors were obtained)
            self.assertEqual(len(film_vectors), 2, "Expected two film vectors.")
            self.assertEqual(len(substrate_vectors), 2, "Expected two substrate vectors.")

            # Example additional checks could include:
            # Checking the orientation of vectors aligns with expected crystal coordinates
            # This placeholder asserts are for illustrative purposes
            self.assertTrue(film_vectors, "Film vectors should not be empty.")
            self.assertTrue(substrate_vectors, "Substrate vectors should not be empty.")

    # Add additional tests here to cover more functionality or edge cases as necessary

if __name__ == "__main__":
    unittest.main()

@jinlhr542 how did you create your PR? usually people clone the repo, make local edits, push them to a fork and make a PR. if you add commits to your fork, they'll show up in this PR.

For revising a already submitted PR, I noticed the following locally based method:

$ gh pr checkout 3572

We can find the above prompt as represented below:

image

you can also use GH web and edit your PR online by simply pressing . while on this page

I haven't used this feature yet, but it sounds very convenient. Below are some related introductions:

GitHub provides a convenient feature for users to quickly edit files and manage their pull requests (PRs) directly from the web interface. By simply pressing the . key while on the PR page (or any repository page), GitHub opens a web-based version of Visual Studio Code (VS Code) in your browser. This environment allows for a more powerful and flexible editing experience without leaving your web browser.

Here’s how you can use this feature to edit your PR online:

Navigate to Your PR: First, go to the GitHub page for your pull request.

Activate the Editor: Press the . key on your keyboard. The page should reload, transforming your browser tab into a VS Code-like environment. If this is your first time using this feature, GitHub might take a moment to initialize the environment.

Edit Files: In this online editor, you can browse through your repository’s file structure, edit files, and make the necessary changes related to your PR. You can take advantage of VS Code’s features, such as search, replace, syntax highlighting, and even some extensions.

Commit Changes: Once you’ve made your edits, you can commit the changes directly from this environment. To do so, save your changes (Ctrl + S or Cmd + S on Mac), then open the Source Control panel on the left sidebar (it looks like a branching icon). You can stage your changes, write a commit message, and then commit by clicking the checkmark icon at the top of the Source Control panel.

Push Changes: If your PR is from a branch in the same repository, the changes will automatically be part of the PR once committed. If your PR is from a fork, you may need to push the changes to your fork (... menu at the top of the Source Control panel, then "Push") and the PR will automatically update to include your new commits.

Return to GitHub: After you’ve made and committed your changes, you can close the editor and return to the GitHub PR page to review and manage your PR as usual. You may want to add comments, request reviews, or perform other PR management tasks.

This feature significantly streamlines the process of editing and managing pull requests by integrating a powerful editor directly into the GitHub interface, making it easier for contributors to make quick adjustments or resolve merge conflicts without switching contexts.

)

for s in substrate_millers:
substrate_slab = SlabGenerator(self.substrate, s, 20, 15, primitive=False).get_slab()
substrate_vectors = reduce_vectors(substrate_slab.lattice.matrix[0], substrate_slab.lattice.matrix[1])
for s_miller in substrate_millers:
substrate_slab = SlabGenerator(substrate, s_miller, 20, 15, primitive=False).get_slab()
substrate_vectors = reduce_vectors(
substrate_slab.oriented_unit_cell.lattice.matrix[0],
substrate_slab.oriented_unit_cell.lattice.matrix[1],
)

vector_sets.append((film_vectors, substrate_vectors, f, s))
vector_sets.append((film_vectors, substrate_vectors, f_miller, s_miller))

return vector_sets

def calculate(
self,
film,
substrate,
film: Structure,
substrate: Structure,
elasticity_tensor=None,
film_millers=None,
substrate_millers=None,
film_millers: ArrayLike = None,
substrate_millers: ArrayLike = None,
ground_state_energy=0,
lowest=False,
):
Expand All @@ -148,33 +159,28 @@ def calculate(
ground state energy are provided:

Args:
film(Structure): conventional standard structure for the film
substrate(Structure): conventional standard structure for the
film (Structure): conventional standard structure for the film
substrate (Structure): conventional standard structure for the
substrate
elasticity_tensor(ElasticTensor): elasticity tensor for the film
elasticity_tensor (ElasticTensor): elasticity tensor for the film
in the IEEE orientation
film_millers(array): film facets to consider in search as defined by
film_millers (array): film facets to consider in search as defined by
miller indices
substrate_millers(array): substrate facets to consider in search as
substrate_millers (array): substrate facets to consider in search as
defined by miller indices
ground_state_energy(float): ground state energy for the film
lowest(bool): only consider lowest matching area for each surface
ground_state_energy (float): ground state energy for the film
lowest (bool): only consider lowest matching area for each surface
"""
self.film = film
self.substrate = substrate

# Generate miller indices if none specified for film
if film_millers is None:
film_millers = sorted(get_symmetrically_distinct_miller_indices(self.film, self.film_max_miller))
film_millers = sorted(get_symmetrically_distinct_miller_indices(film, self.film_max_miller))

# Generate miller indices if none specified for substrate
if substrate_millers is None:
substrate_millers = sorted(
get_symmetrically_distinct_miller_indices(self.substrate, self.substrate_max_miller)
)
substrate_millers = sorted(get_symmetrically_distinct_miller_indices(substrate, self.substrate_max_miller))

# Check each miller index combination
surface_vector_sets = self.generate_surface_vectors(film_millers, substrate_millers)
surface_vector_sets = self.generate_surface_vectors(film, substrate, film_millers, substrate_millers)
for [
film_vectors,
substrate_vectors,
Expand Down
4 changes: 2 additions & 2 deletions pymatgen/analysis/interfaces/zsl.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ def generate_sl_transformation_sets(self, film_area, substrate_area):
lattices with a maximum area

Args:
film_area(int): the unit cell area for the film
substrate_area(int): the unit cell area for the substrate
film_area (int): the unit cell area for the film
substrate_area (int): the unit cell area for the substrate

Returns:
transformation_sets: a set of transformation_sets defined as:
Expand Down
2 changes: 1 addition & 1 deletion pymatgen/analysis/piezo_sensitivity.py
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ def get_rand_IST(self, max_force=1):
symmetry and the acoustic sum rule.

Args:
max_force(float): maximum born effective charge value
max_force (float): maximum born effective charge value

Return:
InternalStrainTensor object
Expand Down
2 changes: 1 addition & 1 deletion pymatgen/analysis/xas/spectrum.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def stitch(self, other: XAS, num_samples: int = 500, mode: Literal["XAFS", "L23"

Args:
other: Another XAS object.
num_samples(int): Number of samples for interpolation.
num_samples (int): Number of samples for interpolation.
mode("XAFS" | "L23"): Either XAFS mode for stitching XANES and EXAFS
or L23 mode for stitching L2 and L3.

Expand Down
2 changes: 1 addition & 1 deletion pymatgen/io/aims/inputs.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ def get_aims_control_parameter_str(self, key: str, value: Any, fmt: str) -> str:
fmt (str): The format string to apply to the value

Returns:
The line to add to the control.in file
str: The line to add to the control.in file
"""
return f"{key:35s}{fmt % value}\n"

Expand Down
6 changes: 3 additions & 3 deletions pymatgen/io/lobster/inputs.py
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ def _get_potcar_symbols(POTCAR_input: str) -> list:
Will return the name of the species in the POTCAR.

Args:
POTCAR_input(str): string to potcar file
POTCAR_input (str): string to potcar file

Returns:
list of the names of the species in string format
Expand Down Expand Up @@ -662,8 +662,8 @@ def standard_calculations_from_vasp_files(
Will generate Lobsterin with standard settings.

Args:
POSCAR_input(str): path to POSCAR
INCAR_input(str): path to INCAR
POSCAR_input (str): path to POSCAR
INCAR_input (str): path to INCAR
POTCAR_input (str): path to POTCAR
dict_for_basis (dict): can be provided: it should look the following:
dict_for_basis={"Fe":'3p 3d 4s 4f', "C": '2s 2p'} and will overwrite all settings from POTCAR_input
Expand Down
2 changes: 1 addition & 1 deletion pymatgen/io/vasp/sets.py
Original file line number Diff line number Diff line change
Expand Up @@ -2162,7 +2162,7 @@ class MVLGBSet(DictSet):
Class for writing a vasp input files for grain boundary calculations, slab or bulk.

Args:
structure(Structure): provide the structure
structure (Structure): provide the structure
k_product: Kpoint number * length for a & b directions, also for c direction in
bulk calculations. Default to 40.
slab_mode (bool): Defaults to False. Use default (False) for a bulk supercell.
Expand Down
32 changes: 25 additions & 7 deletions tests/analysis/interfaces/test_substrate_analyzer.py
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
from __future__ import annotations

from numpy.testing import assert_allclose

from pymatgen.analysis.elasticity.elastic import ElasticTensor
from pymatgen.analysis.interfaces.substrate_analyzer import SubstrateAnalyzer
from pymatgen.symmetry.analyzer import SpacegroupAnalyzer
from pymatgen.util.testing import PymatgenTest

VO2 = PymatgenTest.get_structure("VO2")
TiO2 = PymatgenTest.get_structure("TiO2")

def test_substrate_analyzer_init():
# Film VO2
film = SpacegroupAnalyzer(PymatgenTest.get_structure("VO2"), symprec=0.1).get_conventional_standard_structure()
# Film VO2
film = SpacegroupAnalyzer(VO2, symprec=0.1).get_conventional_standard_structure()
# Substrate TiO2
substrate = SpacegroupAnalyzer(TiO2, symprec=0.1).get_conventional_standard_structure()

# Substrate TiO2
substrate = SpacegroupAnalyzer(
PymatgenTest.get_structure("TiO2"), symprec=0.1
).get_conventional_standard_structure()

def test_substrate_analyzer_init():
film_elastic_tensor = ElasticTensor.from_voigt(
[
[324.32, 187.3, 170.92, 0, 0, 0],
Expand All @@ -32,3 +34,19 @@ def test_substrate_analyzer_init():
assert len(matches) == 296
for match in matches:
assert isinstance(match.match_area, float)


def test_generate_surface_vectors():
film_miller_indices = [(1, 0, 0)]
substrate_miller_indices = [(1, 1, 1)]

vector_sets = SubstrateAnalyzer().generate_surface_vectors(
film, substrate, film_miller_indices, substrate_miller_indices
)
assert len(vector_sets) == 1
film_vectors, substrate_vectors, film_millers, substrate_millers = vector_sets[0]

assert [film_millers] == film_miller_indices
assert [substrate_millers] == substrate_miller_indices
assert_allclose(film_vectors, [[0, 0, 3.035429], [-2.764654e-16, 4.515023, 2.764654e-16]], atol=1e-6)
assert_allclose(substrate_vectors, [[-3.766937, -1.928326, -6.328967], [3.766937, -12.307154, 0.0]], atol=1e-6)
Loading