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.

Sign up for GitHub

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

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

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bf3c728
Fix the problem that the film vectors and substrate vectors were not …
jinlhr542 Jan 23, 2024
8ef1963
pre-commit auto-fixes
pre-commit-ci[bot] Jan 23, 2024
64c37be
breaking: make film + substrate args of generate_surface_vectors()
janosh Feb 8, 2024
193107a
add test_generate_surface_vectors
janosh Feb 8, 2024
de70b7a
fix missing doc str white space
janosh Feb 8, 2024
b9463b7
Merge branch 'master' into pr/jinlhr542/3572
janosh Feb 8, 2024
332a355
loosen atol in test_generate_surface_vectors
janosh Feb 8, 2024
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
9 changes: 7 additions & 2 deletions pymatgen/analysis/interfaces/substrate_analyzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,16 @@ def generate_surface_vectors(self, film_millers, substrate_millers):

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])
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 •
edited
Loading

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 •
edited
Loading

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])
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))

Expand Down