Open AbdullahKazi500 opened 1 month ago
Hi @AbdullahKazi500 , There are good ideas here. But there is also a lot of unnecessary (and confusing) redefinitions and changes that are not pertinent to the issue at hand.
Also, to avoid future confusion, please work on this PR going forward. I will close the rest.
As a first start:
If you need guidance how to do this, happy to point you to materials.
@peterkomar-aws Hi peter thanks for the guidance I am new to open source before this I use to push demo code into master branches for my previous projects I have worked as an experimentalist researcher for 4 years and had my formal education in experimental validation research so I hope its okay its my first time I am doing an opensource hackathon I will try following your guidelines but please do accept my apologies for a lot of different PRs
Hi @AbdullahKazi500 , no worries. We were all there some time in our journey. Please try to follow the contributions guidelines. You can start by creating a fork of the repo, and work there on a feature branch first. Once your tests are passing locally and you are happy with the code, you can create a PR from your fork to this repo.
Hi @peterkomar-aws I have cleaned up and properly formatted the code to enhance readability and maintainability. Here are the key changes made:
Added all necessary imports at the beginning of the code. Defined a custom exception DiscretizationError. Organized and added necessary data classes (LatticeGeometry, DiscretizationProperties, AtomArrangementItem) and enums (SiteType). Implemented validation methods inside AtomArrangementItem to check coordinates and site types. Added docstrings to methods for better understanding. Implemented methods for adding coordinates, getting coordinate lists, iterating, getting length, and discretizing arrangements. Added factory methods for creating different lattice structures (square, rectangular, decorated Bravais, honeycomb, and Bravais lattices). made the PR in the same Patch also removed unnecessary redefinitions
Hi @AbdullahKazi500 , thanks for continuing the work.
You wrote that you removed unnecessary definitions, but the atom_arrangement.py file in your latest commit still contains many repeated redefinitions. E.g. the original defition of AtomArrangementItem is defined in L41, then redefined in L187, L351, and L393; AtomArrangement is originally defined in L63, then redefined in L208, L428.
Your proposed version of the module has 527 lines, I understand that may be difficult to handle, but to move forward with this work, I ask you to review each line of change and addition that you are proposing (here) and carefully consider its function and value. Once you feel that your work is ready for a careful review please click the ready for review button to flip back the PR from draft stage to open. Thank you.
Hi @peterkomar-aws Thanks to the review I will check and modify this again but after testing it locally I am kind of getting the desired results here but still I will try addressing the feedbacks you have advised
Imports Added from future import annotations to ensure forward compatibility with future Python releases. Imported necessary modules and classes (Iterator, dataclass, Decimal, Enum, Number, Union, Tuple, List, np, and shapely.geometry).
FILLED = "filled"
VACANT = "vacant"
This Enum defines two possible types for sites: FILLED and VACANT.
class AtomArrangementItem:
coordinate: Tuple[Number, Number]
site_type: SiteType
This dataclass represents an item in an atom arrangement with a coordinate and a site type. validate_coordinate: Ensures that the coordinate is a tuple of two numbers. _validate_site_type: Ensures that the site type is either FILLED or VACANT.
self._validate_coordinate()
self._validate_site_type()
Validates the coordinate and site type after initializing an AtomArrangementItem.
self._sites = []
Initializes an empty list to hold atom arrangement items.
self._sites.append(AtomArrangementItem(tuple(coordinate), site_type))
return self
Adds a new site to the arrangement and returns self for method chaining.
return [site.coordinate[coordinate_index] for site in self._sites]
Returns a list of coordinates at the specified index (0 for x-coordinates, 1 for y-coordinates).
return iter(self._sites)
def __len__(self):
return len(self._sites)
Allows iteration over the arrangement and retrieves the number of sites
...````
Creates a discretized version of the atom arrangement based on the provided resolution.
Hi @peterkomar-aws this are what the functions, data classes are
@peterkomar-aws let me know your thoughts
Hi @AbdullahKazi500 , thanks for working on this PR.
In my previous comments, I mentioned that you had many re-definitions in your atom_arrangement.py module. This is still the case. I'd like to start a conversation with you about one particular case, SiteType
, since you explicitly mentioned it in your message above.
The class SiteType
is an enum class.
class SiteType(Enum):
VACANT = "Vacant"
FILLED = "Filled"
class SiteType(Enum):
FILLED = "filled"
VACANT = "vacant"
class SiteType(Enum):
VACANT = "Vacant"
FILLED = "Filled"
class SiteType(Enum):
FILLED = "filled"
EMPTY = "empty"
SiteType.VACANT
is a public attribute in this module; changing/eliminating it would break user code built on top of this class. There should be a very good reason to change this public API. What is your reason?class SiteType(Enum):
FILLED = "filled"
EMPTY = "empty"
Since SiteType
is such a simple class, I wish to have a productive conversation about your plans to change the atom_arrangement module through this lens.
In my previous comments, I mentioned that you had many re-definitions in your atom_arrangement.py module. This is still the case. I'd like to start a conversation with you about one particular case,
SiteType
, since you explicitly mentioned it in your message above.The class
SiteType
is an enum class.
- It is part of the main branch, defined in L28
class SiteType(Enum): VACANT = "Vacant" FILLED = "Filled"
- Its first definition appears in L35 in your code, but this is already different from how it is in the main branch
class SiteType(Enum): FILLED = "filled" VACANT = "vacant"
- Question 1: Why did you change the string values (filled vs Filled), and why did you change the order in which they are listed in the class definition?
- Then, a redefinition appears in L182, which is identical to the definition in the main branch
class SiteType(Enum): VACANT = "Vacant" FILLED = "Filled"
- Question 2: What is the purpose of this code? Why did you not leave the original definition from the main branch in place?
- Then, another redefinition appears in L347, where both the attribute names and the string values are different (empty vs vacant).
class SiteType(Enum): FILLED = "filled" EMPTY = "empty"
- Question 3: What is the purpose of this?
SiteType.VACANT
is a public attribute in this module; changing/eliminating it would break user code built on top of this class. There should be a very good reason to change this public API. What is your reason?- Then, another redefinition appears in L388
class SiteType(Enum): FILLED = "filled" EMPTY = "empty"
- Question 4: Why do you need this? Since this is repeating the same definition again, it seems to serve no purpose.
Since
SiteType
is such a simple class, I wish to have a productive conversation about your plans to change the atom_arrangement module through this lens.
Hi peter thanks for the feedback I will try to address your questions Why did you change the string values (filled vs Filled), and why did you change the order in which they are listed in the class definition? Changing the string values and the order might be a result of different naming conventions or standards being applied at different points in the development. I agree The naming conventions for enums should be consistent to avoid confusion and ensure compatibility. but also The order of definition in an enum class typically does not affect functionality but maintaining a consistent order can improve readability.
What is the purpose of this code? Why did you not leave the original definition from the main branch in place? This redefinition might have been unintentionally introduced or left over from a refactoring attempt. thanks for catching this maybe a mistake on my end
Redefinition at Line 347 (Changing VACANT to EMPTY) What is the purpose of this? SiteType.VACANT is a public attribute in this module; changing/eliminating it would break user code built on top of this class. There should be a very good reason to change this public API. What is your reason? aligning with broader naming conventions across the project
Redefinition at Line 388 Why do you need this? Since this is repeating the same definition again, it seems to serve no purpose. This redefinition seems redundant and unnecessary. It likely serves no functional purpose and could be removed to avoid confusion. I think it can be removed
so further plans would be-------------
Removimg the redundant redefinition at line 388. Ensure only one consistent definition of SiteType exists in the module. Overall Plan for SiteType Retain the original definition of SiteType at the beginning of the module:
from enum import Enum
class SiteType(Enum):
VACANT = "Vacant"
FILLED = "Filled"
Remove all other redefinitions to avoid confusion and ensure consistency. Ensure all references to SiteType within the module and any dependent modules use this single definition. Communicate changes clearly if any modifications to the public API are necessary, and provide a migration path if needed. let me know if there is something else that needs to be done
Hi @AbdullahKazi500 , this sounds like a good next step.
Hi @AbdullahKazi500 , this sounds like a good next step.
Hi peter before I make a PR I would like you to review the code if I am going wrong here feel free to correct me
#
# Licensed under the Apache License, Version 2.0 (the "License"). You
# may not use this file except in compliance with the License. A copy of
# the License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
# ANY KIND, either express or implied. See the License for the specific
# language governing permissions and limitations under the License.
from __future__ import annotations
from collections.abc import Iterator
from dataclasses import dataclass
from decimal import Decimal
from enum import Enum
from numbers import Number
from typing import Union, Tuple, List
import numpy as np
from shapely.geometry import Point, Polygon
# Define SiteType enum
class SiteType(Enum):
VACANT = "Vacant"
FILLED = "Filled"
@dataclass
class AtomArrangementItem:
"""Represents an item (coordinate and metadata) in an atom arrangement."""
coordinate: Tuple[Number, Number]
site_type: SiteType
def _validate_coordinate(self) -> None:
if len(self.coordinate) != 2:
raise ValueError(f"{self.coordinate} must be of length 2")
for idx, num in enumerate(self.coordinate):
if not isinstance(num, Number):
raise TypeError(f"{num} at position {idx} must be a number")
def _validate_site_type(self) -> None:
allowed_site_types = {SiteType.FILLED, SiteType.VACANT}
if self.site_type not in allowed_site_types:
raise ValueError(f"{self.site_type} must be one of {allowed_site_types}")
def __post_init__(self) -> None:
self._validate_coordinate()
self._validate_site_type()
class AtomArrangement:
def __init__(self):
"""Represents a set of coordinates that can be used as a register to an AnalogHamiltonianSimulation."""
self._sites = []
def add(self, coordinate: Union[Tuple[Number, Number], np.ndarray], site_type: SiteType = SiteType.FILLED) -> "AtomArrangement":
"""Add a coordinate to the atom arrangement.
Args:
coordinate (Union[tuple[Number, Number], ndarray]): The coordinate of the atom (in meters).
site_type (SiteType): The type of site. Optional. Default is FILLED.
Returns:
AtomArrangement: returns self (to allow for chaining).
"""
self._sites.append(AtomArrangementItem(tuple(coordinate), site_type))
return self
def coordinate_list(self, coordinate_index: Number) -> List[Number]:
"""Returns all the coordinates at the given index.
Args:
coordinate_index (Number): The index to get for each coordinate.
Returns:
List[Number]: The list of coordinates at the given index.
Example:
To get a list of all x-coordinates: coordinate_list(0)
To get a list of all y-coordinates: coordinate_list(1)
"""
return [site.coordinate[coordinate_index] for site in self._sites]
def __iter__(self) -> Iterator:
return iter(self._sites)
def __len__(self):
return len(self._sites)
def discretize(self, properties: 'DiscretizationProperties') -> "AtomArrangement":
"""Creates a discretized version of the atom arrangement, rounding all site coordinates to the closest multiple of the resolution. The types of the sites are unchanged.
Args:
properties (DiscretizationProperties): Capabilities of a device that represent the
resolution with which the device can implement the parameters.
Raises:
DiscretizationError: If unable to discretize the program.
Returns:
AtomArrangement: A new discretized atom arrangement.
"""
try:
position_res = properties.lattice.geometry.positionResolution
discretized_arrangement = AtomArrangement()
for site in self._sites:
new_coordinates = tuple(
round(Decimal(c) / position_res) * position_res for c in site.coordinate
)
discretized_arrangement.add(new_coordinates, site.site_type)
return discretized_arrangement
except Exception as e:
raise DiscretizationError(f"Failed to discretize register {e}") from e
# Factory methods for lattice structures
@classmethod
def from_square_lattice(cls, lattice_constant: float, canvas_boundary_points: List[Tuple[float, float]]) -> "AtomArrangement":
"""Create an atom arrangement with a square lattice."""
arrangement = cls()
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
x_range = np.arange(x_min, x_max, lattice_constant)
y_range = np.arange(y_min, y_max, lattice_constant)
for x in x_range:
for y in y_range:
arrangement.add((x, y))
return arrangement
@classmethod
def from_rectangular_lattice(cls, dx: float, dy: float, canvas_boundary_points: List[Tuple[float, float]]) -> "AtomArrangement":
"""Create an atom arrangement with a rectangular lattice."""
arrangement = cls()
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
for x in np.arange(x_min, x_max, dx):
for y in np.arange(y_min, y_max, dy):
arrangement.add((x, y))
return arrangement
@classmethod
def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> "AtomArrangement":
arrangement = cls()
vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1])
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
i = 0
while (origin := i * vec_a)[0] < x_max:
j = 0
while (point := origin + j * vec_b)[1] < y_max:
if x_min <= point[0] <= x_max and y_min <= point[1] <= y_max:
for dp in decoration_points:
decorated_point = point + np.array(dp)
if x_min <= decorated_point[0] <= x_max and y_min <= decorated_point[1] <= y_max:
arrangement.add(tuple(decorated_point))
j += 1
i += 1
return arrangement
@classmethod
def from_honeycomb_lattice(cls, lattice_constant: float, canvas_boundary_points: List[Tuple[float, float]]) -> "AtomArrangement":
a1 = (lattice_constant, 0)
a2 = (lattice_constant / 2, lattice_constant * np.sqrt(3) / 2)
decoration_points = [(0, 0), (lattice_constant / 2, lattice_constant * np.sqrt(3) / 6)]
return cls.from_decorated_bravais_lattice([a1, a2], decoration_points, canvas_boundary_points)
@classmethod
def from_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> "AtomArrangement":
return cls.from_decorated_bravais_lattice(lattice_vectors, [(0, 0)], canvas_boundary_points)
@dataclass
class LatticeGeometry:
positionResolution: Decimal
@dataclass
class DiscretizationProperties:
lattice: LatticeGeometry
class DiscretizationError(Exception):
pass
# RectangularCanvas class
class RectangularCanvas:
def __init__(self, bottom_left: Tuple[float, float], top_right: Tuple[float, float]):
self.bottom_left = bottom_left
self.top_right = top_right
def is_within(self, point: Tuple[float, float]) -> bool:
x_min, y_min = self.bottom_left
x_max, y_max = self.top_right
x, y = point
return x_min <= x <= x_max and y_min <= y <= y_max
# Example usage
if __name__ == "__main__":
canvas_boundary_points = [(0, 0), (7.5e-5, 0), (7.5e-5, 7.5e-5), (0, 7.5e-5)]
# Create lattice structures
square_lattice = AtomArrangement.from_square_lattice(4e-6, canvas_boundary_points)
rectangular_lattice = AtomArrangement.from_rectangular_lattice(3e-6, 2e-6, canvas_boundary_points)
decorated_bravais_lattice = AtomArrangement.from_decorated_bravais_lattice([(4e-6, 0), (0, 4e-6)], [(1e-6, 1e-6)], canvas_boundary_points)
honeycomb_lattice = AtomArrangement.from_honeycomb_lattice(4e-6, canvas_boundary_points)
bravais_lattice = AtomArrangement.from_bravais_lattice([(4e-6, 0), (0, 4e-6)], canvas_boundary_points)
# Validation function
def validate_lattice(arrangement, lattice_type):
"""Validate the lattice structure."""
num_sites = len(arrangement)
min_distance = None
for i, atom1 in enumerate(arrangement):
for j, atom2 in enumerate(arrangement):
if i != j:
distance = np.linalg.norm(np.array(atom1.coordinate) - np.array(atom2.coordinate))
if min_distance is None or distance < min_distance:
min_distance = distance
print(f"Lattice Type: {lattice_type}")
print(f"Number of lattice points: {num_sites}")
print(f"Minimum distance between lattice points: {min_distance:.2e} meters")
print("-" * 40)
# Validate lattice structures
validate_lattice(square_lattice, "Square Lattice")
validate_lattice(rectangular_lattice, "Rectangular Lattice")
validate_lattice(decorated_bravais_lattice, "Decorated Bravais Lattice")
validate_lattice(honeycomb_lattice, "Honeycomb Lattice")
validate_lattice(bravais_lattice, "Bravais Lattice")
Hi @AbdullahKazi500 , this looks very clean. Please make the edit on the AbdullahKazi500:AbdullahKazi500-patch-8 branch. Then I can provide line-by-line review as part of this PR.
Hi @peterkomar-aws made a PR Recently here here
Hi @AbdullahKazi500 , thanks for adding the changes to the AHS.py file. Since you contribution will be modifications to the atom_arrangement.py file, please add the changes there. If you replace the content of that file with what you have in you AHS.py, then I can do a more through review, beacause I will see the changes with respect to the original atom_arrangement.py file. Thanks.
Hi @AbdullahKazi500 , thanks for adding the changes to the AHS.py file. Since you contribution will be modifications to the atom_arrangement.py file, please add the changes there. If you replace the content of that file with what you have in you AHS.py, then I can do a more through review, beacause I will see the changes with respect to the original atom_arrangement.py file. Thanks.
Okay peter I will make both separate and in file PRs
@peterkomar-aws Okay done now
Great start. You have the right structure for the factory methods.
The main thing that needs change is the implementation of the
from_decorated_bravais_lattice
method. Currently it cuts off too many sites, sites which would fit on the canvas. See my comment for line 141. This is not easy to solve. Looking forward to your idea how to make this method work for arbitrary vec_a and vec_b values.The goal is the fill up the entire canvas, and only remove sites that would truly fall outside of the canvas boundary.
Hi @peterkomar-aws before adding this to the PR May I know is this what you want me to implement so that I know I am going the right way
@classmethod
def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement:
"""Create an atom arrangement with a decorated Bravais lattice."""
arrangement = cls()
vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1])
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
# Calculate the bounds of the lattice within the canvas
lattice_x_min = max(0, x_min)
lattice_x_max = min(x_max, x_max - vec_a[0] + vec_b[0])
lattice_y_min = max(0, y_min)
lattice_y_max = min(y_max, y_max - vec_a[1] + vec_b[1])
for i in np.arange(lattice_x_min, lattice_x_max, vec_a[0]):
for j in np.arange(lattice_y_min, lattice_y_max, vec_b[1]):
origin = i * vec_a + j * vec_b
for dp in decoration_points:
decorated_point = origin + np.array(dp)
if x_min <= decorated_point[0] <= x_max and y_min <= decorated_point[1] <= y_max:
arrangement.add(tuple(decorated_point))
return arrangement
Hi @AbdullahKazi500 , can you explain the logic behind lattice_x_max = min(x_max, x_max - vec_a[0] + vec_b[0])
? (It is very suspicious that vec_a and vec_b are not treated symmetrically by this expression. Logically, they are interchangeable.)
Hi @AbdullahKazi500 , thanks for your contributions. Looking forward to your contributions to this work and repo. Thank you. (I mark the PR draft again; feel free to add commits to it an open it again if you think it's ready for another round of reviews. Thanks.)
Hi @peterkomar-aws since the Organizers has posted this message
Hackers and Maintainers,
We have received a lot of requests over the past few days to extend the review timeline for PRs that are almost ready but not yet. As such, we are extending the review time period by 1 week to June 26th.
Maintainers, if there are PRs that are very close to finalized and significant work has been done to warrant the bounty, please assign and close the unitaryHACK issue by June 26th and you can continue working with the participant until the code is ready to merge.
Thank you, everyone! Congratulations on all the hard work put in so far!
I would like to keep going on this issue and I would like to know your review on this also I am trying to address a few of your comments
can you explain the logic behind
lattice_x_max = min(x_max, x_max - vec_a[0] + vec_b[0])
? (It is very suspicious that vec_a and vec_b are not treated symmetrically by this expression. Logically, they are interchangeable.)
The expression lattice_x_max = min(x_max, x_max - vec_a[0] + vec_b[0]) is used to calculate the maximum bound for the x-coordinates of the lattice within the canvas. This expression, however, does seem asymmetrical and possibly incorrect due to the fact that vec_a and vec_b should be treated symmetrically.
Lattice Vectors (vec_a and vec_b):
vec_a and vec_b are the lattice vectors defining the Bravais lattice. In a decorated Bravais lattice, these vectors are used to generate the lattice points within the canvas boundary. Canvas Boundaries (x_min, y_min, x_max, y_max):
These values define the rectangular area within which the lattice should be generated. Suspicious Expression:
lattice_x_max = min(x_max, x_max - vec_a[0] + vec_b[0]) This expression seems to limit the maximum x-coordinate based on the difference and sum of the components of vec_a and vec_b. Symmetry Issue:
Logically, vec_a and vec_b are interchangeable as they both define the periodicity of the lattice. The expression should be symmetric in terms of the treatment of vec_a and vec_b. Potential Problems:
The expression x_max - vec_a[0] + vec_b[0] lacks symmetry and might produce incorrect bounds. Typically, the bounds should be calculated considering the periodicity and overlap of lattice vectors in both directions (x and y).
a more well defined logic would be
@classmethod
def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement:
"""Create an atom arrangement with a decorated Bravais lattice."""
arrangement = cls()
vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1])
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
# Iterate over lattice points within the canvas boundaries
i = 0
while True:
origin = i * vec_a
if origin[0] > x_max:
break
j = 0
while True:
point = origin + j * vec_b
if point[1] > y_max:
break
if x_min <= point[0] <= x_max and y_min <= point[1] <= y_max:
for dp in decoration_points:
decorated_point = point + np.array(dp)
if x_min <= decorated_point[0] <= x_max and y_min <= decorated_point[1] <= y_max:
arrangement.add(tuple(decorated_point))
j += 1
i += 1
return arrangement
Used nested while loops to iterate over lattice points generated by vec_a and vec_b. Ensured points are within the canvas boundaries before adding them to the arrangement. Symmetric Bound Check:
Calculate the origin point using i vec_a. Calculate the additional lattice point using j vec_b. Check both x and y coordinates to ensure they lie within the canvas boundaries before adding decoration points.
Hi @AbdullahKazi500 , the implementation you proposed in your previous comment,
@classmethod
def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement:
"""Create an atom arrangement with a decorated Bravais lattice."""
arrangement = cls()
vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1])
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
# Iterate over lattice points within the canvas boundaries
i = 0
while True:
origin = i * vec_a
if origin[0] > x_max:
break
j = 0
while True:
point = origin + j * vec_b
if point[1] > y_max:
break
if x_min <= point[0] <= x_max and y_min <= point[1] <= y_max:
for dp in decoration_points:
decorated_point = point + np.array(dp)
if x_min <= decorated_point[0] <= x_max and y_min <= decorated_point[1] <= y_max:
arrangement.add(tuple(decorated_point))
j += 1
i += 1
return arrangement
has the same issue which I pointed out previously, namely
To fill in the entire canvas, depending on the values of vec_a and vec_b, it may not be enough to start from i=0 and j=0. E.g. if vec_a = (1,1) and vec_b = (-1, 1), your current implementation leaves a large empty triangle at the lower right side on a rectangular canvas.
More specifically, running the following test
lattice_vectors = (
(1, 1),
(-1, 1),
)
decoration_points = (
(0, 0),
)
canvas_boundary_points = (
(0, 0),
(10, 0),
(10, 10),
(0, 10),
)
register = AtomArrangement.from_decorated_bravais_lattice(lattice_vectors, decoration_points, canvas_boundary_points)
produces a register
which looks like this
Hi @AbdullahKazi500 , the implementation you proposed in your previous comment,
@classmethod def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement: """Create an atom arrangement with a decorated Bravais lattice.""" arrangement = cls() vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1]) x_min, y_min = canvas_boundary_points[0] x_max, y_max = canvas_boundary_points[2] # Iterate over lattice points within the canvas boundaries i = 0 while True: origin = i * vec_a if origin[0] > x_max: break j = 0 while True: point = origin + j * vec_b if point[1] > y_max: break if x_min <= point[0] <= x_max and y_min <= point[1] <= y_max: for dp in decoration_points: decorated_point = point + np.array(dp) if x_min <= decorated_point[0] <= x_max and y_min <= decorated_point[1] <= y_max: arrangement.add(tuple(decorated_point)) j += 1 i += 1 return arrangement
has the same issue which I pointed out previously, namely
To fill in the entire canvas, depending on the values of vec_a and vec_b, it may not be enough to start from i=0 and j=0. E.g. if vec_a = (1,1) and vec_b = (-1, 1), your current implementation leaves a large empty triangle at the lower right side on a rectangular canvas.
More specifically, running the following test
lattice_vectors = ( (1, 1), (-1, 1), ) decoration_points = ( (0, 0), ) canvas_boundary_points = ( (0, 0), (10, 0), (10, 10), (0, 10), ) register = AtomArrangement.from_decorated_bravais_lattice(lattice_vectors, decoration_points, canvas_boundary_points)
produces a
register
which looks like this
it seems like it is not covering over the canvas
class SiteType(Enum):
VACANT = "Vacant"
FILLED = "Filled"
@dataclass
class AtomArrangementItem:
coordinate: Tuple[Number, Number]
site_type: SiteType
def _validate_coordinate(self) -> None:
if len(self.coordinate) != 2:
raise ValueError(f"{self.coordinate} must be of length 2")
for idx, num in enumerate(self.coordinate):
if not isinstance(num, Number):
raise TypeError(f"{num} at position {idx} must be a number")
def _validate_site_type(self) -> None:
allowed_site_types = {SiteType.FILLED, SiteType.VACANT}
if self.site_type not in allowed_site_types:
raise ValueError(f"{self.site_type} must be one of {allowed_site_types}")
def __post_init__(self) -> None:
self._validate_coordinate()
self._validate_site_type()
class AtomArrangement:
def __init__(self):
self._sites = []
def add(self, coordinate: Union[Tuple[Number, Number], np.ndarray], site_type: SiteType = SiteType.FILLED) -> AtomArrangement:
self._sites.append(AtomArrangementItem(tuple(coordinate), site_type))
return self
def coordinate_list(self, coordinate_index: Number) -> List[Number]:
return [site.coordinate[coordinate_index] for site in self._sites]
def __iter__(self) -> Iterator:
return iter(self._sites)
def __len__(self):
return len(self._sites)
def discretize(self, properties: DiscretizationProperties) -> AtomArrangement:
try:
position_res = properties.lattice.positionResolution
discretized_arrangement = AtomArrangement()
for site in self._sites:
new_coordinates = tuple(
round(Decimal(c) / position_res) * position_res for c in site.coordinate
)
discretized_arrangement.add(new_coordinates, site.site_type)
return discretized_arrangement
except Exception as e:
raise DiscretizationError(f"Failed to discretize register {e}") from e
@classmethod
def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement:
"""Create an atom arrangement with a decorated Bravais lattice."""
if len(lattice_vectors) != 2 or any(len(vec) != 2 for vec in lattice_vectors):
raise ValueError("Invalid lattice vectors provided.")
if any(len(dp) != 2 for dp in decoration_points):
raise ValueError("Invalid decoration points provided.")
if len(canvas_boundary_points) != 4:
raise ValueError("Invalid canvas boundary points provided.")
arrangement = cls()
vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1])
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
def is_within_canvas(point: np.ndarray) -> bool:
return x_min <= point[0] <= x_max and y_min <= point[1] <= y_max
# Determine the step directions and limits
n_steps_x = int(np.ceil((x_max - x_min) / np.linalg.norm(vec_a)))
n_steps_y = int(np.ceil((y_max - y_min) / np.linalg.norm(vec_b)))
for i in range(-n_steps_x, n_steps_x + 1):
for j in range(-n_steps_y, n_steps_y + 1):
origin = i * vec_a + j * vec_b
for dp in decoration_points:
decorated_point = origin + np.array(dp)
if is_within_canvas(decorated_point):
arrangement.add(tuple(decorated_point))
return arrangement
@dataclass
class LatticeGeometry:
positionResolution: Decimal
@dataclass
class DiscretizationProperties:
lattice: LatticeGeometry
class DiscretizationError(Exception):
pass
# Example usage
if __name__ == "__main__":
lattice_vectors = (
(1, 1),
(-1, 1),
)
decoration_points = (
(0, 0),
)
canvas_boundary_points = (
(0, 0),
(10, 0),
(10, 10),
(0, 10),
)
register = AtomArrangement.from_decorated_bravais_lattice(lattice_vectors, decoration_points, canvas_boundary_points)
# Validation function
def validate_lattice(arrangement):
num_sites = len(arrangement)
min_distance = None
for i, atom1 in enumerate(arrangement):
for j, atom2 in enumerate(arrangement):
if i != j:
distance = np.linalg.norm(np.array(atom1.coordinate) - np.array(atom2.coordinate))
if min_distance is None or distance < min_distance:
min_distance = distance
print(f"Number of lattice points: {num_sites}")
print(f"Minimum distance between lattice points: {min_distance:.2e} meters")
print("-" * 40)
validate_lattice(register)
I have tried modifying the code
Hi @AbdullahKazi500 , regarding these lines in your proposal,
n_steps_x = int(np.ceil((x_max - x_min) / np.linalg.norm(vec_a)))
n_steps_y = int(np.ceil((y_max - y_min) / np.linalg.norm(vec_b)))
can you explain why n_steps_x
is computed only from x_max
and x_min
, while you are using it to set a hard limit on how many times you step with vec_a
later? See here
for i in range(-n_steps_x, n_steps_x + 1):
for j in range(-n_steps_y, n_steps_y + 1):
origin = i * vec_a + j * vec_b
and why n_step_y
is computed only from y_max
and y_min
, while you are using it to set a hard limit on how many times you step step with vec_b
?
Any implementation that does not treat vec_a
and vec_b
symmetrically is very suspicious, because if I reorder the lattice_vectors
input list (which effectively swaps the values of vec_a
and vec_b
), there is a good chance the returned AtomArrangement will be different.
regarding these lines in your proposal,
Hi @peterkomar-aws When computing n_steps_x and n_steps_y, we want to ensure that both lattice vectors vec_a and vec_b are treated symmetrically is that what you mean but swapping the order of vec_a and vec_b in lattice_vectors will it affect the outcome of AtomArrangement.
Hi @AbdullahKazi500 , regarding these lines in your proposal,
n_steps_x = int(np.ceil((x_max - x_min) / np.linalg.norm(vec_a))) n_steps_y = int(np.ceil((y_max - y_min) / np.linalg.norm(vec_b)))
can you explain why
n_steps_x
is computed only fromx_max
andx_min
, while you are using it to set a hard limit on how many times you step withvec_a
later? See herefor i in range(-n_steps_x, n_steps_x + 1): for j in range(-n_steps_y, n_steps_y + 1): origin = i * vec_a + j * vec_b
and why
n_step_y
is computed only fromy_max
andy_min
, while you are using it to set a hard limit on how many times you step step withvec_b
?Any implementation that does not treat
vec_a
andvec_b
symmetrically is very suspicious, because if I reorder thelattice_vectors
input list (which effectively swaps the values ofvec_a
andvec_b
), there is a good chance the returned AtomArrangement will be different.
so if Both n_steps_x and n_steps_y are now calculated based on the maximum difference between canvas boundaries (x_max - x_min and y_max - y_min). will this ensures that the steps taken in both directions (for vec_a and vec_b) are sufficient to cover the entire canvas area. and for loops now if we use n_steps_x and n_steps_y symmetrically for both vec_a and vec_b. does This guarantees that the lattice arrangement fills the canvas correctly regardless of the order of vec_a and vec_b in lattice_vectors what do you think of this changes
class AtomArrangementItem:
coordinate: Tuple[Number, Number]
site_type: SiteType
def _validate_coordinate(self) -> None:
if len(self.coordinate) != 2:
raise ValueError(f"{self.coordinate} must be of length 2")
for idx, num in enumerate(self.coordinate):
if not isinstance(num, Number):
raise TypeError(f"{num} at position {idx} must be a number")
def _validate_site_type(self) -> None:
allowed_site_types = {SiteType.FILLED, SiteType.VACANT}
if self.site_type not in allowed_site_types:
raise ValueError(f"{self.site_type} must be one of {allowed_site_types}")
def __post_init__(self) -> None:
self._validate_coordinate()
self._validate_site_type()
class AtomArrangement:
def __init__(self):
self._sites = []
def add(self, coordinate: Union[Tuple[Number, Number], np.ndarray], site_type: SiteType = SiteType.FILLED) -> AtomArrangement:
self._sites.append(AtomArrangementItem(tuple(coordinate), site_type))
return self
def coordinate_list(self, coordinate_index: Number) -> List[Number]:
return [site.coordinate[coordinate_index] for site in self._sites]
def __iter__(self) -> Iterator:
return iter(self._sites)
def __len__(self):
return len(self._sites)
def discretize(self, properties: DiscretizationProperties) -> AtomArrangement:
try:
position_res = properties.lattice.positionResolution
discretized_arrangement = AtomArrangement()
for site in self._sites:
new_coordinates = tuple(
round(Decimal(c) / position_res) * position_res for c in site.coordinate
)
discretized_arrangement.add(new_coordinates, site.site_type)
return discretized_arrangement
except Exception as e:
raise DiscretizationError(f"Failed to discretize register {e}") from e
@classmethod
def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement:
"""Create an atom arrangement with a decorated Bravais lattice."""
if len(lattice_vectors) != 2 or any(len(vec) != 2 for vec in lattice_vectors):
raise ValueError("Invalid lattice vectors provided.")
if any(len(dp) != 2 for dp in decoration_points):
raise ValueError("Invalid decoration points provided.")
if len(canvas_boundary_points) != 4:
raise ValueError("Invalid canvas boundary points provided.")
arrangement = cls()
vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1])
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
def is_within_canvas(point: np.ndarray) -> bool:
return x_min <= point[0] <= x_max and y_min <= point[1] <= y_max
# Determine the step directions and limits symmetrically
n_steps_x = int(np.ceil(max(x_max - x_min, y_max - y_min) / np.linalg.norm(vec_a)))
n_steps_y = int(np.ceil(max(x_max - x_min, y_max - y_min) / np.linalg.norm(vec_b)))
for i in range(-n_steps_x, n_steps_x + 1):
for j in range(-n_steps_y, n_steps_y + 1):
origin = i * vec_a + j * vec_b
for dp in decoration_points:
decorated_point = origin + np.array(dp)
if is_within_canvas(decorated_point):
arrangement.add(tuple(decorated_point))
return arrangement
@dataclass
class LatticeGeometry:
positionResolution: Decimal
@dataclass
class DiscretizationProperties:
lattice: LatticeGeometry
class DiscretizationError(Exception):
pass
# Example usage
if __name__ == "__main__":
lattice_vectors = (
(1, 1),
(-1, 1),
)
decoration_points = (
(0, 0),
)
canvas_boundary_points = (
(0, 0),
(10, 0),
(10, 10),
(0, 10),
)
register = AtomArrangement.from_decorated_bravais_lattice(lattice_vectors, decoration_points, canvas_boundary_points)
# Validation function
def validate_lattice(arrangement):
num_sites = len(arrangement)
min_distance = None
for i, atom1 in enumerate(arrangement):
for j, atom2 in enumerate(arrangement):
if i != j:
distance = np.linalg.norm(np.array(atom1.coordinate) - np.array(atom2.coordinate))
if min_distance is None or distance < min_distance:
min_distance = distance
print(f"Number of lattice points: {num_sites}")
print(f"Minimum distance between lattice points: {min_distance:.2e} meters")
print("-" * 40)
validate_lattice(register)
Hi @AbdullahKazi500 , I appreciate that this implementation is symmetric in its treatment of vec_a
and vec_b
, this is a good first step. The implementation you are proposing in your comment above, namely,
@classmethod
def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement:
"""Create an atom arrangement with a decorated Bravais lattice."""
if len(lattice_vectors) != 2 or any(len(vec) != 2 for vec in lattice_vectors):
raise ValueError("Invalid lattice vectors provided.")
if any(len(dp) != 2 for dp in decoration_points):
raise ValueError("Invalid decoration points provided.")
if len(canvas_boundary_points) != 4:
raise ValueError("Invalid canvas boundary points provided.")
arrangement = cls()
vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1])
x_min, y_min = canvas_boundary_points[0]
x_max, y_max = canvas_boundary_points[2]
def is_within_canvas(point: np.ndarray) -> bool:
return x_min <= point[0] <= x_max and y_min <= point[1] <= y_max
# Determine the step directions and limits symmetrically
n_steps_x = int(np.ceil(max(x_max - x_min, y_max - y_min) / np.linalg.norm(vec_a)))
n_steps_y = int(np.ceil(max(x_max - x_min, y_max - y_min) / np.linalg.norm(vec_b)))
for i in range(-n_steps_x, n_steps_x + 1):
for j in range(-n_steps_y, n_steps_y + 1):
origin = i * vec_a + j * vec_b
for dp in decoration_points:
decorated_point = origin + np.array(dp)
if is_within_canvas(decorated_point):
arrangement.add(tuple(decorated_point))
return arrangement
also does not properly fill out the canvas for the test I proposed above, i.e.
lattice_vectors = (
(1, 1),
(-1, 1),
)
decoration_points = (
(0, 0),
)
canvas_boundary_points = (
(0, 0),
(10, 0),
(10, 10),
(0, 10),
)
register = AtomArrangement.from_decorated_bravais_lattice(lattice_vectors, decoration_points, canvas_boundary_points)
produces a register
that looks like this
I suspect this is due to the premature stopping of the for i
and for j
loops, i.e. n_steps_x
and n_steps_y
are too small. My guess is this is due to the fact that you are computing them by dividing the Euclidean norm of vec_a
and vec_b
, which is generally larger than the projection of the said vectors in the x and y directions, respectively.
Hi @AbdullahKazi500 , the implementation you proposed in your previous comment,
@classmethod def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement: """Create an atom arrangement with a decorated Bravais lattice.""" arrangement = cls() vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1]) x_min, y_min = canvas_boundary_points[0] x_max, y_max = canvas_boundary_points[2] # Iterate over lattice points within the canvas boundaries i = 0 while True: origin = i * vec_a if origin[0] > x_max: break j = 0 while True: point = origin + j * vec_b if point[1] > y_max: break if x_min <= point[0] <= x_max and y_min <= point[1] <= y_max: for dp in decoration_points: decorated_point = point + np.array(dp) if x_min <= decorated_point[0] <= x_max and y_min <= decorated_point[1] <= y_max: arrangement.add(tuple(decorated_point)) j += 1 i += 1 return arrangement
has the same issue which I pointed out previously, namely
To fill in the entire canvas, depending on the values of vec_a and vec_b, it may not be enough to start from i=0 and j=0. E.g. if vec_a = (1,1) and vec_b = (-1, 1), your current implementation leaves a large empty triangle at the lower right side on a rectangular canvas.
More specifically, running the following test
lattice_vectors = ( (1, 1), (-1, 1), ) decoration_points = ( (0, 0), ) canvas_boundary_points = ( (0, 0), (10, 0), (10, 10), (0, 10), ) register = AtomArrangement.from_decorated_bravais_lattice(lattice_vectors, decoration_points, canvas_boundary_points)
produces a
register
which looks like thisit seems like it is not covering over the canvas
class SiteType(Enum): VACANT = "Vacant" FILLED = "Filled" @dataclass class AtomArrangementItem: coordinate: Tuple[Number, Number] site_type: SiteType def _validate_coordinate(self) -> None: if len(self.coordinate) != 2: raise ValueError(f"{self.coordinate} must be of length 2") for idx, num in enumerate(self.coordinate): if not isinstance(num, Number): raise TypeError(f"{num} at position {idx} must be a number") def _validate_site_type(self) -> None: allowed_site_types = {SiteType.FILLED, SiteType.VACANT} if self.site_type not in allowed_site_types: raise ValueError(f"{self.site_type} must be one of {allowed_site_types}") def __post_init__(self) -> None: self._validate_coordinate() self._validate_site_type() class AtomArrangement: def __init__(self): self._sites = [] def add(self, coordinate: Union[Tuple[Number, Number], np.ndarray], site_type: SiteType = SiteType.FILLED) -> AtomArrangement: self._sites.append(AtomArrangementItem(tuple(coordinate), site_type)) return self def coordinate_list(self, coordinate_index: Number) -> List[Number]: return [site.coordinate[coordinate_index] for site in self._sites] def __iter__(self) -> Iterator: return iter(self._sites) def __len__(self): return len(self._sites) def discretize(self, properties: DiscretizationProperties) -> AtomArrangement: try: position_res = properties.lattice.positionResolution discretized_arrangement = AtomArrangement() for site in self._sites: new_coordinates = tuple( round(Decimal(c) / position_res) * position_res for c in site.coordinate ) discretized_arrangement.add(new_coordinates, site.site_type) return discretized_arrangement except Exception as e: raise DiscretizationError(f"Failed to discretize register {e}") from e @classmethod def from_decorated_bravais_lattice(cls, lattice_vectors: List[Tuple[float, float]], decoration_points: List[Tuple[float, float]], canvas_boundary_points: List[Tuple[float, float]]) -> AtomArrangement: """Create an atom arrangement with a decorated Bravais lattice.""" if len(lattice_vectors) != 2 or any(len(vec) != 2 for vec in lattice_vectors): raise ValueError("Invalid lattice vectors provided.") if any(len(dp) != 2 for dp in decoration_points): raise ValueError("Invalid decoration points provided.") if len(canvas_boundary_points) != 4: raise ValueError("Invalid canvas boundary points provided.") arrangement = cls() vec_a, vec_b = np.array(lattice_vectors[0]), np.array(lattice_vectors[1]) x_min, y_min = canvas_boundary_points[0] x_max, y_max = canvas_boundary_points[2] def is_within_canvas(point: np.ndarray) -> bool: return x_min <= point[0] <= x_max and y_min <= point[1] <= y_max # Determine the step directions and limits n_steps_x = int(np.ceil((x_max - x_min) / np.linalg.norm(vec_a))) n_steps_y = int(np.ceil((y_max - y_min) / np.linalg.norm(vec_b))) for i in range(-n_steps_x, n_steps_x + 1): for j in range(-n_steps_y, n_steps_y + 1): origin = i * vec_a + j * vec_b for dp in decoration_points: decorated_point = origin + np.array(dp) if is_within_canvas(decorated_point): arrangement.add(tuple(decorated_point)) return arrangement @dataclass class LatticeGeometry: positionResolution: Decimal @dataclass class DiscretizationProperties: lattice: LatticeGeometry class DiscretizationError(Exception): pass # Example usage if __name__ == "__main__": lattice_vectors = ( (1, 1), (-1, 1), ) decoration_points = ( (0, 0), ) canvas_boundary_points = ( (0, 0), (10, 0), (10, 10), (0, 10), ) register = AtomArrangement.from_decorated_bravais_lattice(lattice_vectors, decoration_points, canvas_boundary_points) # Validation function def validate_lattice(arrangement): num_sites = len(arrangement) min_distance = None for i, atom1 in enumerate(arrangement): for j, atom2 in enumerate(arrangement): if i != j: distance = np.linalg.norm(np.array(atom1.coordinate) - np.array(atom2.coordinate)) if min_distance is None or distance < min_distance: min_distance = distance print(f"Number of lattice points: {num_sites}") print(f"Minimum distance between lattice points: {min_distance:.2e} meters") print("-" * 40) validate_lattice(register)
I have tried modifying the code
okay peter I will have a look
Issue #, if available: fixes #969 Description of changes: added lattice.py polygon.py and the factory ahs all in one file Testing done:
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.