-
Notifications
You must be signed in to change notification settings - Fork 24
elphondb.py 3d plots for the gkkp added #92
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
base: bug-fixes
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |||||||||||||||||||
| # | ||||||||||||||||||||
| from yambopy import * | ||||||||||||||||||||
| from netCDF4 import Dataset | ||||||||||||||||||||
| from math import sqrt | ||||||||||||||||||||
| import math | ||||||||||||||||||||
| import numpy as np | ||||||||||||||||||||
| from yambopy.tools.string import marquee | ||||||||||||||||||||
| import os | ||||||||||||||||||||
|
|
@@ -64,6 +64,7 @@ def __init__(self,lattice,filename='ndb.elph_gkkp',folder_gkkp='SAVE',save='SAVE | |||||||||||||||||||
| self.alat = lattice.alat | ||||||||||||||||||||
| self.rlat = lattice.rlat | ||||||||||||||||||||
| self.car_kpoints = lattice.car_kpoints | ||||||||||||||||||||
| self.red_kpoints = lattice.red_kpoints | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Check if databases exist. Exit only if header is missing. | ||||||||||||||||||||
| try: database = Dataset(filename) | ||||||||||||||||||||
|
|
@@ -232,7 +233,7 @@ def get_gkkp_mixed(self): | |||||||||||||||||||
| self.gkkp_mixed = np.real(self.gkkp)*np.real(self.gkkp_bare)+np.imag(self.gkkp)*np.imag(self.gkkp_bare) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @add_fig_kwargs | ||||||||||||||||||||
| def plot_elph(self,data,kcoords=None,plt_show=False,plt_cbar=False,**kwargs): | ||||||||||||||||||||
| def plot_elph_old(self,data,kcoords=None,plt_show=False,plt_cbar=False,**kwargs): | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| 2D scatterplot in the BZ: | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -280,6 +281,163 @@ def plot_elph(self,data,kcoords=None,plt_show=False,plt_cbar=False,**kwargs): | |||||||||||||||||||
|
|
||||||||||||||||||||
| if plt_show: plt.show() | ||||||||||||||||||||
| else: print("Plot ready.\nYou can customise adding savefig, title, labels, text, show, etc...") | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
||||||||||||||||||||
| @add_fig_kwargs |
Copilot
AI
Feb 15, 2026
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 parameter name 'threeD' uses camelCase, which is inconsistent with the Python naming convention (PEP 8) and other parameters in this function (plt_show, plt_cbar, kcoords) which use snake_case. Consider renaming to 'three_d' or 'is_3d' for consistency with codebase conventions.
Copilot
AI
Feb 15, 2026
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 docstring first line says "2D scatterplot in the BZ:" but this is no longer accurate when threeD=True, as the function now creates multiple 2D scatterplots in a subplot grid. Consider updating the first line to something like "Scatterplot(s) in the BZ:" or "2D/3D scatterplot in the BZ:" to better reflect the dual functionality.
| 2D scatterplot in the BZ: | |
| Scatterplot(s) in the BZ: |
Copilot
AI
Feb 15, 2026
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 docstring has inconsistent indentation. Line 290 has extra leading spaces before "(i)" and line 291 has extra leading spaces before "(ii)". This should be aligned consistently with the rest of the docstring (as seen in the original plot_elph_old function).
| (i) in k-space of the quantity A_{k}(iq,inu,ib1,ib2). | |
| (ii) in q-space of the quantity A_{q}(ik,inu,ib1,ib2). | |
| (i) in k-space of the quantity A_{k}(iq,inu,ib1,ib2). | |
| (ii) in q-space of the quantity A_{q}(ik,inu,ib1,ib2). |
Copilot
AI
Feb 15, 2026
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 note "NB: So far requires a 2D system." is outdated since the function now supports 3D systems when threeD=True. Consider updating this to clarify that 2D systems are required only when threeD=False, or remove this note entirely since the next sentence explains the 3D functionality.
| NB: So far requires a 2D system. |
Copilot
AI
Feb 15, 2026
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 variable 'color_map' is assigned but never used in the 2D plotting path. In the original plot_elph_old function (lines 268-269), this variable is also assigned but not used. Consider removing this dead code or using it to set the colormap explicitly for the scatter plot.
| if plt_cbar: | |
| if 'cmap' in kwargs.keys(): color_map = plt.get_cmap(kwargs['cmap']) | |
| else: color_map = plt.get_cmap('viridis') |
Copilot
AI
Feb 15, 2026
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 code uses math.ceil here but np.ceil elsewhere (line 334). For consistency, consider using np.ceil throughout the function, especially since numpy is already imported and used extensively in this codebase. This would also allow removing the 'import math' statement if it's only used for ceil.
| nrows = int(math.ceil(nlay / ncols)) | |
| nrows = int(np.ceil(nlay / ncols)) |
Copilot
AI
Feb 15, 2026
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.
If no unique layers are found (e.g., all k-points have the same value along the selected axis), nlay will be 0, leading to nrows = 0. Calling plt.subplots(0, ncols) will raise a ValueError. Consider adding a check before creating subplots to ensure nlay > 0, and either raise a meaningful error or fall back to 2D plotting.
| nrows = int(math.ceil(nlay / ncols)) | |
| if nlay == 0: | |
| raise ValueError( | |
| "No unique layers found along the selected axis for 3D projection. " | |
| "Try adjusting the tolerance or use the 2D plotting mode (threeD=False)." | |
| ) | |
| nrows = int(np.ceil(float(nlay) / ncols)) |
Copilot
AI
Feb 15, 2026
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.
In the 3D plotting path, the function creates 'axes' (a list of axes) but doesn't set 'self.ax', unlike the 2D path which sets both 'self.fig' and 'self.ax'. This inconsistency could cause issues if users try to access 'self.ax' after a 3D plot, as they do in the tutorial examples (e.g., yelph.ax.set_title()). Consider either setting 'self.ax = axes' for consistency, or documenting that 'self.ax' is not available for 3D plots and users should access axes through 'self.fig.axes' instead.
| axes = np.atleast_1d(axes).ravel() | |
| axes = np.atleast_1d(axes).ravel() | |
| # For consistency with the 2D path (`self.fig, self.ax = plt.subplots(...)`), | |
| # expose the 3D subplot array via `self.ax`. | |
| self.ax = axes |
Copilot
AI
Feb 15, 2026
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 masking operation uses kpts_r (rounded values) to select points but then uses the mask to index the original kpts array. This is correct behavior for selecting the original k-points that belong to a layer. However, consider adding a check to ensure that at least one point exists in each layer before attempting to plot it, to avoid potential errors with empty arrays.
| mask = (kpts_r == layer_val) | |
| mask = (kpts_r == layer_val) | |
| # If no k-points fall into this layer, skip plotting to avoid empty-array issues | |
| if not np.any(mask): | |
| continue |
Copilot
AI
Feb 15, 2026
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 3D plotting path doesn't return self.fig, while the 2D path has an explicit return statement at line 371. For API consistency and to match the expected behavior of the @add_fig_kwargs decorator (which should be added), this function should return self.fig at the end of the 3D path, similar to how the original plot_elph_old relies on the decorator to return the figure.
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 red_kpoints attribute is assigned from lattice.red_kpoints but is never used anywhere in the code. If this was intended for the 3D plotting functionality, it should be utilized, or the line should be removed if it's not needed. Consider clarifying whether this is intended for future use or if it should be removed.