-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add calc_displacements_ellipsoid
function
#87
base: develop
Are you sure you want to change the base?
Conversation
@coderabbitai review |
Actions performedReview triggered.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes to the Changes
Poem
Tip Early access features: disabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (9)
doped/utils/displacements.py (9)
604-604
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- marker=dict(color="black", size=3), + marker={"color": "black", "size": 3},Tools
Ruff
604-604: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
609-611
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- scene=dict( - aspectmode="data", - ), + scene={"aspectmode": "data"},Tools
Ruff
609-611: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
638-638
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- line=dict(color="black", width=2), + line={"color": "black", "width": 2},Tools
Ruff
638-638: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
744-753
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- marker=dict( - size=10, - opacity=0.5, - color=anisotropy_info_df["the_longest_radius"], # Set color according to column "a" - colorscale="rainbow", - colorbar=dict( - title="The longest radius of ellipsoid", - titleside="right", - ), - ), + marker={ + "size": 10, + "opacity": 0.5, + "color": anisotropy_info_df["the_longest_radius"], # Set color according to column "a" + "colorscale": "rainbow", + "colorbar": { + "title": "The longest radius of ellipsoid", + "titleside": "right", + }, + },Tools
Ruff
744-753: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
749-752: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
761-761
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- line=dict(color="black", dash="dash"), + line={"color": "black", "dash": "dash"},Tools
Ruff
761-761: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
771-771
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- title_font=dict(size=10), + title_font={"size": 10},Tools
Ruff
771-771: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
772-772
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- tickfont=dict(size=12), + tickfont={"size": 12},Tools
Ruff
772-772: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
782-782
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- title_font=dict(size=10), + title_font={"size": 10},Tools
Ruff
782-782: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
783-783
: Optimize dictionary creation.Rewrite the
dict
call as a literal for better performance and readability.- tickfont=dict(size=12), + tickfont={"size": 12},Tools
Ruff
783-783: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- doped/utils/displacements.py (3 hunks)
Additional context used
Ruff
doped/utils/displacements.py
604-604: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
609-611: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
638-638: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
744-753: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
749-752: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
761-761: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
771-771: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
772-772: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
782-782: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
783-783: Unnecessary
dict
call (rewrite as a literal)Rewrite as a literal
(C408)
Additional comments not posted (2)
doped/utils/displacements.py (2)
35-35
: LGTM!The changes to
calc_site_displacements
improve clarity and modularity. The function is well-structured and handles edge cases appropriately.
985-985
: LGTM!The repositioning of
_get_bulk_struct_with_defect
improves the modularity of the code. The function is well-documented and handles different defect types appropriately.
@teruya7 this looks very nice, thank you very much for adding to the code! 🙌 Some small things on the checklist to get this merged:
|
Also following the CodeRabbit suggestions above; could you change the |
@kavanase |
@teruya7 that was quick! 🙌 |
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.
Added two code requests there!
Once done, I think this is all ready to be merged 😃
Thanks again @teruya7!
@@ -126,6 +126,22 @@ def test_plot_site_displacements_error(self): | |||
with pytest.raises(ValueError): | |||
defect_entry.plot_site_displacements(vector_to_project_on=[0, 0, 1], relative_to_defect=True) | |||
|
|||
def test_calc_displacements_ellipsoid(self): |
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.
Nice!
Could you add some quick image comparison tests (for the plotting functions, using matplotlib
) here too, like the tests at the bottom of this module?
For this you just need to add @custom_mpl_image_compare(filename="...")
above the test function as for the others, where filename
is the name of the reference file to compare to in tests/data/remote_baseline_plots
, and your test function returns the matplotlib
Figure
object. Would be good to do some quick tests for the different possible combinations of options.
Then maybe a quick test each like you've done here for the outputs of the function with a substitution and interstitial defect please? (Best to test each type of defect when possible). There's an example of this in test_calc_site_displacements
bulk_sc, defect_sc_with_site, defect_site_index = _get_bulk_struct_with_defect(defect_entry) | ||
|
||
# Map sites in defect supercell to bulk supercell | ||
mappings = get_site_mapping_indices(defect_sc_with_site, bulk_sc) | ||
mappings_dict = {i[1]: i[2] for i in mappings} # {defect_sc_index: bulk_sc_index} | ||
# Loop over sites in defect sc | ||
disp_dict = { # mapping defect site index (in defect sc) to displacement | ||
"Index (defect)": [], | ||
"Species": [], | ||
"Species_with_index": [], | ||
"Displacement": [], | ||
"Displacement Norm": [], | ||
"Distance to defect": [], | ||
"X sites in cartesian coordinate (defect)": [], | ||
"Y sites in cartesian coordinate (defect)": [], | ||
"Z sites in cartesian coordinate (defect)": [], | ||
} # type: dict | ||
|
||
sc_defect_frac_coords = _get_defect_supercell_bulk_site_coords(defect_entry) | ||
if sc_defect_frac_coords is None: | ||
raise ValueError( | ||
"The relaxed defect position (`DefectEntry.sc_defect_frac_coords`) has not been parsed. " | ||
"Please use `DefectsParser`/`DefectParser` to parse relaxed defect positions before " | ||
"calculating site displacements." | ||
) | ||
|
||
for i, site in enumerate(defect_sc_with_site): | ||
bulk_sc_index = mappings_dict[i] # Map to bulk sc | ||
bulk_site = bulk_sc[bulk_sc_index] # Get site in bulk sc | ||
# Calculate displacement (need to account for pbc!) | ||
# First final point, then initial point | ||
frac_disp = pbc_diff(site.frac_coords, bulk_site.frac_coords) # in fractional coords | ||
disp = bulk_sc.lattice.get_cartesian_coords(frac_disp) # in Angstroms | ||
# Distance to defect site (last site in defect sc) | ||
distance = defect_sc_with_site.get_distance(i, defect_site_index) # len(defect_sc_with_site) - 1) | ||
|
||
disp_dict["Index (defect)"].append(i) | ||
disp_dict["Displacement"].append(disp) | ||
disp_dict["Displacement Norm"].append(np.linalg.norm(disp, ord=2)) | ||
disp_dict["Distance to defect"].append(distance) | ||
disp_dict["Species_with_index"].append(f"{site.specie.name}({i})") | ||
disp_dict["Species"].append(site.specie.name) | ||
disp_dict["X sites in cartesian coordinate (defect)"].append( | ||
_shift_defect_site_to_center_of_the_supercell( | ||
site.frac_coords, sc_defect_frac_coords, bulk_sc | ||
)[0] | ||
) | ||
disp_dict["Y sites in cartesian coordinate (defect)"].append( | ||
_shift_defect_site_to_center_of_the_supercell( | ||
site.frac_coords, sc_defect_frac_coords, bulk_sc | ||
)[1] | ||
) | ||
disp_dict["Z sites in cartesian coordinate (defect)"].append( | ||
_shift_defect_site_to_center_of_the_supercell( | ||
site.frac_coords, sc_defect_frac_coords, bulk_sc | ||
)[2] | ||
) | ||
|
||
with warnings.catch_warnings(): | ||
warnings.filterwarnings("ignore", "invalid value encountered in scalar divide") | ||
|
||
# sort each list in disp dict by index of species in bulk element list, then by distance to defect: | ||
element_list = [ | ||
el.symbol for el in defect_entry.defect.structure.composition.elements | ||
] # host elements | ||
element_list += sorted( | ||
[ # extrinsic elements, sorted alphabetically for deterministic ordering in output: | ||
el.symbol | ||
for el in defect_entry.defect.defect_structure.composition.elements | ||
if el.symbol not in element_list | ||
] | ||
) | ||
# Combine the lists into a list of tuples, then sort, then unpack: | ||
combined = list(zip(*disp_dict.values())) | ||
combined.sort( | ||
key=lambda x: (element_list.index(x[1]), x[4], x[0]) | ||
) # Sort by species, then distance, then index | ||
|
||
( | ||
disp_dict["Index (defect)"], | ||
disp_dict["Species"], | ||
disp_dict["Species_with_index"], | ||
disp_dict["Displacement"], | ||
disp_dict["Displacement Norm"], | ||
disp_dict["Distance to defect"], | ||
disp_dict["X sites in cartesian coordinate (defect)"], | ||
disp_dict["Y sites in cartesian coordinate (defect)"], | ||
disp_dict["Z sites in cartesian coordinate (defect)"], | ||
) = zip(*combined) |
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.
A good chunk of this code is duplicated from the calc_site_displacements
function. It would be best for code efficiency and maintenance if this is pulled out into a helper function that then is used in both functions (with flags for relative_to_defect
, vector_to_project_on
and ellipsoid
to determine which disp_dict
data is needed?).
Could you implement this please?
I also updated the functions to return the |
Thank you for checking! |
No description provided.