New functionality for generating slab consistent bulk model #134

AkashHiregange · 2023-11-14T13:36:55Z

No description provided.

The file name for slab model was incorrect in the previous version. Corrected it

codecov · 2023-11-14T13:45:31Z

Codecov Report

Attention: Patch coverage is 98.38710% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.23%. Comparing base (2dc2644) to head (7f9bcb2).

Files	Patch %	Lines
carmm/build/slab_consistent_bulk_generator.py	97.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   88.03%   88.23%   +0.19%     
==========================================
  Files          78       80       +2     
  Lines        3202     3264      +62     
==========================================
+ Hits         2819     2880      +61     
- Misses        383      384       +1

Flag	Coverage Δ
unittests	`88.23% <98.38%> (+0.19%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OscarvanVuren · 2023-11-14T14:31:10Z

Surely there must be a Numpy function to check if a float is close to an integer within tolerance? Perhaps I expect too much from Numpy though

logsdail

Added some high-level comments. Functionality looks great, my comments are mainly around programming practice.

logsdail · 2023-11-14T14:48:51Z

carmm/build/slab_consistent_bulk_generator.py

+
+# This functionality will help to generate a bulk model from a slab.
+
+from ase import Atoms


Imports should be in functions where used, not at the class level.

Will change it!!

logsdail · 2023-11-14T14:49:48Z

carmm/build/slab_consistent_bulk_generator.py

+from ase import Atoms
+
+
+def is_close_to_integer(arr, tolerance=1e-5):


Is this supposed to be a private routine to help the work flow? I might make this private with _ at the front, and also move down the file (as lower priority than the function below it)

Sounds good. Thanks.

logsdail · 2023-11-14T14:50:37Z

carmm/build/slab_consistent_bulk_generator.py

+    return close_to_integer
+
+
+def bulk_identifier(slab: Atoms, cutoff_distance=10.0):


Is the slab: Atoms definition needed? Can't we just check the type when received?

This would remove the need for the import outside a function

Sure. This will make the code look tidy and more readable I guess. Thanks

carmm/build/slab_consistent_bulk_generator.py

examples/build_slab_consistent_bulk.py

logsdail · 2023-11-14T14:53:33Z

examples/build_slab_consistent_bulk.py

+    from carmm.build.slab_consistent_bulk_generator import bulk_identifier
+    from ase.io import read
+    from ase.formula import Formula
+    slab = read('data/slab_consistent_bulk/geometry_CoO_111_slab_model.in')


Can we use an existing surface model in the repository? No problem to keep this, but just cautious of us introducing a new model for each test.

I thought about that. I didn't find any existing surface model in the examples/data folder of CARMM

You can just use the ASE slab generating functions to create one?

See: https://wiki.fysik.dtu.dk/ase/ase/build/surface.html

That is true. However, the whole idea of writing this script was to generate bulk for surfaces where the basis vectors are transformed while generating desired surfaces (like pymatgen). ASE on the other hand does not transform the basis vectors. Hence, I believe the tests will be reliable if the checks are made against surfaces generated by pymatgen.

I can use the pymatgen functionality to create a slab for test. This might help avoid the use of geometry file altogether

Works for me, though you’ll have to introduce PyMatgen into the testing infrastructure (as currently it is not included).

Also, do you need to state this is the purpose in the function preamble?

I know you have lots of detail, but telling the user what it is supposed to work with is probably very useful.

Yes, that makes sense. I will add a bit more detail to the docstring.

Made the suggested changes.

logsdail · 2023-11-14T15:14:50Z

Ah OK, so perhaps my comment makes no difference. I thought it was a bonding distance cutoff.

________________________________ From: AkashHiregange ***@***.***> Sent: 14 November 2023 15:13 To: logsdail/carmm ***@***.***> Cc: Andrew Logsdail ***@***.***>; Comment ***@***.***> Subject: Re: [logsdail/carmm] New functionality for generating slab consistent bulk model (PR #134) External email to Cardiff University - Take care when replying/opening attachments or links. Nid ebost mewnol o Brifysgol Caerdydd yw hwn - Cymerwch ofal wrth ateb/agor atodiadau neu ddolenni. @AkashHiregange commented on this pull request.

________________________________ In carmm/build/slab_consistent_bulk_generator.py<#134 (comment)>:

+ bulk atoms in a slab structure. This new bulk structure will be consistent with the slab model and the

+ computed bulk energy for this structure will help to obtain surface energies that are convergent with + increasing slab thickness. Further detailed information is available in the following literature article. + Schultz, Peter A. + "First-principles calculations of metal surfaces. I. Slab-consistent bulk reference for convergent surface + properties." Physical Review B 103.19 (2021): 195426. + + How this functionality works: + 1. the difference between the x,y and z coordinates of an atom and its corresponding periodic image will be a linear + combination of the cell vectors a,b and c. + 2. This idea is extended here in case of a slab model where the linear combination check + is considered only w.r.t to the a and b vectors + + Args: + - slab: ASE Atoms object representing the slab structure + - cutoff_distance: Cutoff distance for identifying neighboring atoms (default is 10.0). This will be used to This is the distance that is used to check the co-ordination environment of a given atom. For example, the distances between a given atom and all other atoms that lie within a 10 angstrom radius is checked. But since the atoms at the bottom of the slab have no atoms below it, I had to force it check for atoms that have a z-coordinate value great than current atom that is being checked (dist <= cutoff_distance and slab[ind].position[2] / atom_i.position[2] >= 0.9999]). But, I will have a chat with Harry to look into it — Reply to this email directly, view it on GitHub<#134 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFMK4M7TF7SBS7QYY2CMCBTYEODC5AVCNFSM6AAAAAA7K2JBJKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRZHE4DQOBYG4>. You are receiving this because you commented.Message ID: ***@***.***>

AkashHiregange · 2023-11-14T15:51:50Z

Surely there must be a Numpy function to check if a float is close to an integer within tolerance? Perhaps I expect too much from Numpy though

I tried to search for such functionality within numpy, but couldn't find one.

OscarvanVuren · 2023-11-14T16:58:57Z

I tried to search for such functionality within numpy, but couldn't find one.

Fair. I had a look too and failed to find such a routine.

carmm/build/slab_consistent_bulk_generator.py

logsdail

Added a further comment, overall looks very good.

logsdail · 2024-02-06T20:03:19Z

examples/build_slab_consistent_bulk.py

+    from carmm.build.slab_consistent_bulk_generator import bulk_identifier
+    from ase.io import read
+    from ase.formula import Formula
+    slab = read('data/slab_consistent_bulk/geometry_CoO_111_slab_model.in')


I know you have lots of detail, but telling the user what it is supposed to work with is probably very useful.

…he geometry file from examples folder and modified the test to make use of existing infrastructure.

GaryLZW · 2024-03-08T16:36:42Z

Looks good to me!

New functionality for generating slab consistent bulk model

da4fe46

AkashHiregange requested review from OscarvanVuren, robinsonmt1 and GaryLZW November 14, 2023 13:37

Update build_slab_consistent_bulk.py

6497cdf

The file name for slab model was incorrect in the previous version. Corrected it

logsdail reviewed Nov 14, 2023

View reviewed changes

GaryLZW reviewed Feb 2, 2024

View reviewed changes

carmm/build/slab_consistent_bulk_generator.py Outdated Show resolved Hide resolved

carmm/build/slab_consistent_bulk_generator.py Show resolved Hide resolved

logsdail reviewed Feb 3, 2024

View reviewed changes

carmm/build/slab_consistent_bulk_generator.py Outdated Show resolved Hide resolved

AkashHiregange and others added 2 commits February 6, 2024 10:47

Merge branch 'logsdail:master' into akash-new_bulk_from_slab

7dc9c38

Update 2 made suggested changes with regards to slab copy and indices

31dcd9b

logsdail reviewed Feb 6, 2024

View reviewed changes

AkashHiregange and others added 2 commits February 8, 2024 11:35

Merge branch 'logsdail:master' into akash-new_bulk_from_slab

6f1c014

Updated the docstring with a bit more detailed explanation, removed t…

94a2af1

…he geometry file from examples folder and modified the test to make use of existing infrastructure.

AkashHiregange requested a review from GaryLZW March 4, 2024 11:32

GaryLZW approved these changes Mar 8, 2024

View reviewed changes

Merge branch 'logsdail:main' into akash-new_bulk_from_slab

7f9bcb2

AkashHiregange merged commit c286940 into logsdail:main Mar 8, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New functionality for generating slab consistent bulk model #134

New functionality for generating slab consistent bulk model #134

AkashHiregange commented Nov 14, 2023

codecov bot commented Nov 14, 2023 •

edited

Loading

OscarvanVuren commented Nov 14, 2023

logsdail left a comment

logsdail Nov 14, 2023

AkashHiregange Nov 14, 2023

logsdail Nov 14, 2023

AkashHiregange Nov 14, 2023

logsdail Nov 14, 2023

logsdail Nov 14, 2023

AkashHiregange Nov 14, 2023

logsdail Nov 14, 2023

AkashHiregange Nov 14, 2023

logsdail Feb 3, 2024

logsdail Feb 3, 2024

AkashHiregange Feb 6, 2024 •

edited

Loading

AkashHiregange Feb 6, 2024

logsdail Feb 6, 2024

logsdail Feb 6, 2024

AkashHiregange Feb 7, 2024

AkashHiregange Feb 9, 2024

logsdail commented Nov 14, 2023 via email

AkashHiregange commented Nov 14, 2023

OscarvanVuren commented Nov 14, 2023

logsdail left a comment

logsdail Feb 6, 2024

GaryLZW commented Mar 8, 2024


		# This functionality will help to generate a bulk model from a slab.

		from ase import Atoms

		from ase import Atoms


		def is_close_to_integer(arr, tolerance=1e-5):

		return close_to_integer


		def bulk_identifier(slab: Atoms, cutoff_distance=10.0):

New functionality for generating slab consistent bulk model #134

New functionality for generating slab consistent bulk model #134

Conversation

AkashHiregange commented Nov 14, 2023

codecov bot commented Nov 14, 2023 • edited Loading

Codecov Report

OscarvanVuren commented Nov 14, 2023

logsdail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkashHiregange Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logsdail commented Nov 14, 2023 via email

AkashHiregange commented Nov 14, 2023

OscarvanVuren commented Nov 14, 2023

logsdail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaryLZW commented Mar 8, 2024

codecov bot commented Nov 14, 2023 •

edited

Loading

AkashHiregange Feb 6, 2024 •

edited

Loading