Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: Factory methods for AHS AtomArrangments by AbdullahKazi500 #989
feature: Factory methods for AHS AtomArrangments by AbdullahKazi500 #989
Changes from all commits
63cdf2b
5bcd8b9
05b43a9
ead9f97
bd6f911
ebd1e08
a89c222
ad9b803
b293b94
5dd7853
a9b0701
a800d28
f811125
eabfbb3
ca699aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using Google formatted docstrings in this repo (see more here: https://github.com/google/styleguide/blob/gh-pages/pyguide.md#38-comments-and-docstrings), please don't remove these empty lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for many other removed lines in the docstrings below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterkomar-aws Hi peter resolved all the issues let me know if something is left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this change? What edge case do you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of changing the iteration method in this context is to ensure that the method returns a proper iterator over the _sites list.
return self._sites.iter()
This directly calls the iter method of the list object stored in _sites.
return iter(self._sites)
This uses the built-in iter() function to create an iterator from the list _sites.
Both are valid and functionally equivalent. The main goal is to return an iterator for the _sites list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this change? What edge case do you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have the RectangularCanvas defined, let's pass that as an argument instead of the list of tuples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since square lattice is a subset of rectangular lattices, you can implement this factory method by calling the rectangular lattice constructor with dx = dy = lattice_constant arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add detailed docstrings listing what each is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
[0]
indexing assumes too much about how the points are ordered in the input, it can fail if the user passes the points in different order. Yet another reason to require aRectangularCanvas
object as an argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, let's use the
RectangularCanvas
type as the type of the argument.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add detailed docstrings listing what each is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, let's use the
RectangularCanvas
and add docstrings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few issues with the implementation here:
i=0
andj=0
. E.g. ifvec_a = (1,1)
andvec_b = (-1, 1)
, your current implementation leaves a large empty triangle at the lower right side on a rectangular canvas.point
is inside the canvas. This misses adding decoration points from unit cells whosepoint
variable is outside of the canvas, but the decoration points are inside the canvas boundary.Getting the implementation of this method right is the crux of this issue. We can start from an inefficient but working solution, and worry about efficiency later, but the current implementation is not filling the canvas properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterkomar-aws this is a good catch need to think about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to address this issue we can implement something like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, once you remove the redefinitions of
DiscretizationProperties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not redefine
DiscretizationProperties
in this module, instead use the existing definition from the discretization_types.py module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not redefine
DiscretizationError
in this module, instead use the existing definition from the discretization_types.py module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add docstrigs to this class and its methods