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

[jinlhr542]

added by @janosh: breaking because of changes in 64c37be. SubstrateAnalyzer was poorly designed, requiring the user to know to call SubstrateAnalyzer.calculate before SubstrateAnalyzer.generate_surface_vectors. calling SubstrateAnalyzer().generate_surface_vectors alone would raise due to missing attributes film and substrate.

This change is to help the users to be capable to track the real film vectors and substrate vectors expressed in their own original crystal coordinates. This is important when they make interface structures because they need to figure out the exact crystal vectors in along the a, b, c vectors in the supercell

[janosh]

thanks @jinlhr542! we need a test for this change in behavior

[janosh]

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

[jinlhr542]

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?

[hongyi-zhao]

@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.

[janosh]

@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

[hongyi-zhao]

@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:

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.