feature: Factory methods for AHS AtomArrangments by AbdullahKazi500 #989

AbdullahKazi500 · 2024-06-04T16:04:00Z

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

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

defined the SiteType enum and AtomArrangementItem dataclass to handle the coordinates and site types, ensuring validations. AtomArrangement Class: The class initializes with an empty list of sites. Add Method: Adds a coordinate to the arrangement with a site type. Retrieves a list of coordinates for a given index. Rounds coordinates to the nearest multiple of the given resolution. Factory Methods: Methods for creating square and rectangular lattice arrangements. Demonstrates creating a square lattice with specified boundary points and lattice constant.

fixes amazon-braket#969

peterkomar-aws · 2024-06-05T14:12:25Z

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:

Please remove redefinitions
Please add your test to the test folder in the appropriate module.

If you need guidance how to do this, happy to point you to materials.

AbdullahKazi500 · 2024-06-05T16:32:04Z

@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

peterkomar-aws · 2024-06-05T19:08:47Z

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.

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

AbdullahKazi500 · 2024-06-07T17:57:30Z

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

peterkomar-aws · 2024-06-07T19:15:40Z

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.

AbdullahKazi500 · 2024-06-07T19:20:00Z

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

AbdullahKazi500 · 2024-06-10T16:42:25Z

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.

AbdullahKazi500 · 2024-06-10T16:43:25Z

Hi @peterkomar-aws this are what the functions, data classes are

AbdullahKazi500

needing a review

AbdullahKazi500 · 2024-06-10T16:51:55Z

@peterkomar-aws let me know your thoughts

peterkomar-aws · 2024-06-10T19:50:24Z

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.

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.

AbdullahKazi500 · 2024-06-10T20:19:00Z

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

peterkomar-aws · 2024-06-10T20:43:52Z

Hi @AbdullahKazi500 , this sounds like a good next step.

AbdullahKazi500 · 2024-06-11T13:58:30Z

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

AbdullahKazi500 · 2024-06-11T14:23:59Z

Hi @peterkomar-aws made a PR Recently here here

peterkomar-aws · 2024-06-11T14:24:02Z

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.

AbdullahKazi500 · 2024-06-11T14:25:07Z

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

AbdullahKazi500 · 2024-06-11T14:31:19Z

@peterkomar-aws Okay done now

peterkomar-aws

I see you appended the latest version of the file. Instead, please replace the content of the entire file with this, so I can do a more careful, line-by-line review. Thanks.

peterkomar-aws · 2024-06-11T14:36:37Z

AHS.py

Please remove this file, and make all edits to the atom_arrangement.py module and the test_atom_arrangement.py module.

Hi @peterkomar-aws replaced the content of the entire file with this now going through the comments

The individual file was deleted

peterkomar-aws · 2024-06-11T14:37:53Z