High index surfaces #400

skatnagallu · 2021-10-28T09:35:52Z

A new function to get a high index surface based on the step length, step orientation and kink orientation.

Updating the indentations

coveralls · 2021-10-28T15:13:24Z

Pull Request Test Coverage Report for Build 1713832390

34 of 35 (97.14%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 70.167%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyiron_atomistics/atomistics/structure/factory.py	34	35	97.14%

Totals
Change from base Build 1689646937:	0.07%
Covered Lines:	11614
Relevant Lines:	16552

💛 - Coveralls

sudarsan-surendralal

This looks great @skatnagallu although I think some changes need to be more (see below)

sudarsan-surendralal · 2021-10-28T15:17:22Z

pyiron_atomistics/atomistics/structure/factory.py

@@ -407,3 +407,32 @@ def from_pymatgen(pymatgen_obj):
    @wraps(ovito_to_pyiron)
    def from_ovito(ovito_obj):
        return ovito_to_pyiron(ovito_obj)
+
+    def get_high_ind_surf_sk(self, element='Ni', Xstr='fcc', lattice_constant=3.526, terraceOrientation=[1,1,1],


A more descriptive name like get_high_index_surface would be better

Indeed, with tab completion we don't need to be afraid of the slightly longer name.
I'd go with high_index_surface to better match the pattern of the other factory methods.

We should also think about defaults for element and Xstr (maybe crystalstructure is a better name like for the ASE functions). Our other methods don't have defaults for elements and I'd prefer this to be consistent.

sudarsan-surendralal · 2021-10-28T15:19:40Z

pyiron_atomistics/atomistics/structure/factory.py

+        vec2 = (np.asanyarray(fin_kinkOrientation).dot(lengthTerrace)) + stepDownVector
+        highIndexSurface = np.cross(np.asanyarray(vec1),np.asanyarray(vec2))
+        highIndexSurface = np.array(highIndexSurface/np.gcd.reduce(highIndexSurface), dtype=int)
+        return fin_kinkOrientation, fin_stepOrientation, highIndexSurface


Here, you return the indices of the kink. step and the high index surface. But I guess it would be nice to go one step further and use these quantities to actually return the structure

liamhuber

Cool, very nice contribution! 👍

In addition to Sudarsan's comments:

Please make this PEP8 compliant (capitalization, etc). The easiest way to this is probably just download an IDE, e.g. pycharm, and open the atomistics repository as a project. For pycharm you can then just go through and get rid of all the yellow squiggles, most of which they'll probably have a recommendation for.
Please add a docstring

The final thing is to add a unit test. You're very welcome to do this yourself, but if it's new to you then once the other items have been dealt with I'd be happy to write the test up so you have an example pertaining to content you're super familiar with. Just ping me for re-review when you're ready 😄

liamhuber · 2021-10-28T19:51:04Z

pyiron_atomistics/atomistics/structure/factory.py

@@ -407,3 +407,32 @@ def from_pymatgen(pymatgen_obj):
    @wraps(ovito_to_pyiron)
    def from_ovito(ovito_obj):
        return ovito_to_pyiron(ovito_obj)
+
+    def get_high_ind_surf_sk(self, element='Ni', Xstr='fcc', lattice_constant=3.526, terraceOrientation=[1,1,1],


Indeed, with tab completion we don't need to be afraid of the slightly longer name.
I'd go with high_index_surface to better match the pattern of the other factory methods.

sudarsan-surendralal

This looks good now. Some minor suggestions for improvement below

pyiron_atomistics/atomistics/structure/factory.py

tests/atomistics/structure/test_high_index_surface.py

pyiron_atomistics/atomistics/structure/factory.py

…s a test for length of the slab.

liamhuber

Very nice! I have a couple of nits still, and a test request, but IMO the only "real" outstanding issue is just a design question on how we should handle the return value(s) 😄

tests/atomistics/structure/test_high_index_surface.py

liamhuber · 2021-11-08T13:47:24Z

pyiron_atomistics/atomistics/structure/factory.py

@@ -71,24 +73,29 @@ def aimsgb(self):

    def cut(self, *args, **kwargs):
        return self.ase.cut(*args, **kwargs)
+


I think elsewhere we do the doc addition immediately adjacent to the function definition, so for consistency let's just keep doing that instead of these newlines.

pyiron_atomistics/atomistics/structure/factory.py

Refactored the function into two functions high_index_surface which returns only structure and high_index_surface_info which returns the Miller indices of the surface place, step and kink directions.

liamhuber

Good stuff. I would like to wait to merge until the tests are passing -- I think the current failures relate to issues with hdf5io so this should only require some patience and some merging master into this branch.

pyiron_atomistics/atomistics/structure/factory.py

Refactored the function into two functions high_index_surface which returns only structure and high_index_surface_info which returns the Miller indices of the surface place, step and kink directions. made changes to high_index_surface function to use the given element, crystal_structure and lattice_constant.

Refactored the function into two functions high_index_surface which returns only structure and high_index_surface_info which returns the Miller indices of the surface place, step and kink directions. corrected it to give periodic boundary conditions in all three directions.

sudarsan-surendralal

Apart from a couple of minor changes, LGTM!

pyiron_atomistics/atomistics/structure/factory.py

stale · 2021-12-04T00:58:04Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

…stics into step_surface

Update factory.py

e0bf6ca

A new function to get a high index surface based on the step length, step orientation and kink orientation.

skatnagallu added the enhancement New feature or request label Oct 28, 2021

skatnagallu requested review from sudarsan-surendralal and liamhuber October 28, 2021 09:35

Indentation

96e3ecd

Updating the indentations

sudarsan-surendralal added the good first issue Good for newcomers label Oct 28, 2021

sudarsan-surendralal added 2 commits October 28, 2021 16:46

🐛 Fix formatting

66d35ed

Use crystal function from the class rather than a project

1787dba

sudarsan-surendralal suggested changes Oct 28, 2021

View reviewed changes

liamhuber requested changes Oct 28, 2021

View reviewed changes

skatnagallu added 4 commits October 29, 2021 13:17

Made changes according to the input from the reviewers.

1a786df

Test for get high index surface

becccc3

Changes to the order of instances returned

d09c41f

Changes to the unittest

2ad0764

sudarsan-surendralal reviewed Nov 7, 2021

View reviewed changes

pyiron_atomistics/atomistics/structure/factory.py Outdated Show resolved Hide resolved

tests/atomistics/structure/test_high_index_surface.py Outdated Show resolved Hide resolved

pyiron_atomistics/atomistics/structure/factory.py Outdated Show resolved Hide resolved

Changes to the unittest and the name of the function. Unittest now ha…

66d9d82

…s a test for length of the slab.

liamhuber reviewed Nov 8, 2021

View reviewed changes

Changes to the unittest. Added valueerror assertions.

50b2549

Refactored the function into two functions high_index_surface which returns only structure and high_index_surface_info which returns the Miller indices of the surface place, step and kink directions.

liamhuber reviewed Nov 11, 2021

View reviewed changes

niklassiemer reviewed Nov 11, 2021

View reviewed changes

pyiron_atomistics/atomistics/structure/factory.py Outdated Show resolved Hide resolved

skatnagallu added 2 commits November 11, 2021 11:51

sudarsan-surendralal approved these changes Nov 18, 2021

View reviewed changes

pyiron_atomistics/atomistics/structure/factory.py Outdated Show resolved Hide resolved

pyiron_atomistics/atomistics/structure/factory.py Outdated Show resolved Hide resolved

stale bot added the stale label Dec 4, 2021

stale bot closed this Dec 18, 2021

sudarsan-surendralal reopened this Jan 18, 2022

stale bot removed the stale label Jan 18, 2022

Merge remote-tracking branch 'origin/master' into step_surface

10fb35e

sudarsan-surendralal added 3 commits January 18, 2022 17:38

Apply suggestions from code review

3cd2981

Merge branch 'step_surface' of https://github.com/pyiron/pyiron_atomi…

cb01f35

…stics into step_surface

Remove default values

af372f6

sudarsan-surendralal merged commit ada9fdb into master Jan 18, 2022

delete-merged-branch bot deleted the step_surface branch January 18, 2022 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

High index surfaces #400

High index surfaces #400

skatnagallu commented Oct 28, 2021

coveralls commented Oct 28, 2021 •

edited

Loading

sudarsan-surendralal left a comment

sudarsan-surendralal Oct 28, 2021

liamhuber Oct 28, 2021

pmrv Oct 29, 2021

sudarsan-surendralal Oct 28, 2021

liamhuber left a comment

liamhuber Oct 28, 2021

sudarsan-surendralal left a comment

liamhuber left a comment

liamhuber Nov 8, 2021

liamhuber left a comment

sudarsan-surendralal left a comment

stale bot commented Dec 4, 2021

		@@ -71,24 +73,29 @@ def aimsgb(self):

		def cut(self, args, *kwargs):
		return self.ase.cut(args, *kwargs)

High index surfaces #400

High index surfaces #400

Conversation

skatnagallu commented Oct 28, 2021

coveralls commented Oct 28, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1713832390

💛 - Coveralls

sudarsan-surendralal left a comment

Choose a reason for hiding this comment

sudarsan-surendralal Oct 28, 2021

Choose a reason for hiding this comment

liamhuber Oct 28, 2021

Choose a reason for hiding this comment

pmrv Oct 29, 2021

Choose a reason for hiding this comment

sudarsan-surendralal Oct 28, 2021

Choose a reason for hiding this comment

liamhuber left a comment

Choose a reason for hiding this comment

liamhuber Oct 28, 2021

Choose a reason for hiding this comment

sudarsan-surendralal left a comment

Choose a reason for hiding this comment

liamhuber left a comment

Choose a reason for hiding this comment

liamhuber Nov 8, 2021

Choose a reason for hiding this comment

liamhuber left a comment

Choose a reason for hiding this comment

sudarsan-surendralal left a comment

Choose a reason for hiding this comment

stale bot commented Dec 4, 2021

coveralls commented Oct 28, 2021 •

edited

Loading