-
Notifications
You must be signed in to change notification settings - Fork 21
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
✨ SiQAD coordinate iteration #223
Conversation
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.
clang-tidy made some suggestions
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 94.31% 94.42% +0.11%
==========================================
Files 82 82
Lines 7406 7431 +25
==========================================
+ Hits 6985 7017 +32
+ Misses 421 414 -7
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Functionality-wise, everything looks nice—just a couple of nit-picky comments regarding structure.
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.
clang-tidy made some suggestions
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
clang-tidy review says "All clean, LGTM! 👍" |
The volume of a SiQAD coodinate returns (x + 1) * (y + 1) * (z * 1) like the other coordinate types. Perhaps this should be reviewed. I left the SiQAD coordinates out of the coordinate types to test the volume for. |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
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.
LGTM 👍 Many thanks once again for your effort!
Description
This PR extracts
coord_iterator
from theoffset
namespace and instead allows for specialisations depending on theCoordinateType
template argument. The default implementation for offset coordinates is left in tact, and a specialisation for SiQAD coordinates is added. Additionally,print_charge_layout
is updated to use this new way of iteration over SiQAD coordinates.Lastly, a mistake in the documentation of
coordinate
,ground_coordinate
,foreach_coordinate
, andforeach_ground_coordinate
was found: the end bound was stated to be inclusive, but this is not the case. I have added(exclusive)
in order to avoid further confusion.Checklist: