Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 160 additions & 2 deletions yambopy/dbs/elphondb.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
self.red_kpoints = lattice.red_kpoints

Copilot uses AI. Check for mistakes.

# Check if databases exist. Exit only if header is missing.
try: database = Dataset(filename)
Expand Down Expand Up @@ -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:

Expand Down Expand Up @@ -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...")

Copy link

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new plot_elph function is missing the @add_fig_kwargs decorator that was present in the original plot_elph_old function. This decorator provides important functionality for handling common figure kwargs like 'title', 'show', and 'savefig'. Without it, users who pass these arguments will experience unexpected behavior as the kwargs won't be processed.

Suggested change
@add_fig_kwargs

Copilot uses AI. Check for mistakes.
def plot_elph(self, data, kcoords=None, plt_show=False, plt_cbar=False,
threeD=False, axis='z', tol=1e-6, ncols=3, **kwargs):
Copy link

Copilot AI Feb 15, 2026

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 uses AI. Check for mistakes.
"""
2D scatterplot in the BZ:
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
2D scatterplot in the BZ:
Scatterplot(s) in the BZ:

Copilot uses AI. Check for mistakes.

(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).
Comment on lines +290 to +291
Copy link

Copilot AI Feb 15, 2026

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

Suggested change
(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 uses AI. Check for mistakes.

Any real quantity which is a function of only the k-grid or q-grid may be supplied.

The indices iq/ik,inu,ib1,ib2 are user-specified.

- kcoords refers to the k/q-grid in Cartesian coordinates (i.e., yelph.car_qpoints and similar).
If None is specified, a k-space, fixed-q plot is assumed.

- if plt_show plot is shown
- if plt_cbar colorbar is shown
- kwargs example: marker='H', s=300, cmap='viridis', etc.

NB: So far requires a 2D system.
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
NB: So far requires a 2D system.

Copilot uses AI. Check for mistakes.
If threeD=True, plots BZ planes at constant component along `axis`
(x/y/z) in a subplot grid.
"""

# --- select k-points as in original code ---
if kcoords is None:
kpts = self.car_kpoints # Assume k-space plot
else:
kpts = kcoords # Plot on momentum map supplied by user

# --- input check as in original code ---
if len(data) != len(kpts):
raise ValueError('Something wrong in data dimensions (%d data vs %d kpts)' % (len(data), len(kpts)))

# -------------------------------------------------------------------------
# Helper(s) for 3D layer selection
# -------------------------------------------------------------------------
def _axis_to_index(ax):
ax = ax.lower()
if ax == 'x': return 0
if ax == 'y': return 1
if ax == 'z': return 2
raise ValueError("axis must be one of 'x', 'y', 'z'")

def _unique_with_tol(vals, tol_):
vals = np.asarray(vals)
decimals = max(0, int(np.ceil(-np.log10(tol_))))
vals_r = np.round(vals, decimals=decimals)
uniq = np.unique(vals_r)
return uniq, vals_r, decimals

ax_idx = _axis_to_index(axis)

# -------------------------------------------------------------------------
# 2D path: keep original structure/behavior
# -------------------------------------------------------------------------
if not threeD:
# Global plot stuff (original)
self.fig, self.ax = plt.subplots(1, 1)
self.ax.add_patch(BZ_Wigner_Seitz(self.lattice))

if plt_cbar:
if 'cmap' in kwargs.keys(): color_map = plt.get_cmap(kwargs['cmap'])
else: color_map = plt.get_cmap('viridis')

Comment on lines +346 to +349
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
if plt_cbar:
if 'cmap' in kwargs.keys(): color_map = plt.get_cmap(kwargs['cmap'])
else: color_map = plt.get_cmap('viridis')

Copilot uses AI. Check for mistakes.
lim = 1.05 * np.linalg.norm(self.rlat[0])
self.ax.set_xlim(-lim, lim)
self.ax.set_ylim(-lim, lim)

# Reproduce plot also in adjacent BZs (original)
BZs = shifted_grids_2D(kpts, self.rlat)
for kpts_s in BZs:
plot = self.ax.scatter(kpts_s[:, 0], kpts_s[:, 1], c=data, **kwargs)

if plt_cbar:
self.fig.colorbar(plot)

plt.gca().set_aspect('equal')

if plt_show:
plt.show()
else:
print("Plot ready.\nYou can customise adding savefig, title, labels, text, show, etc...")
return

# -------------------------------------------------------------------------
# 3D path: make a subplot grid of constant-axis slices (projections)
# -------------------------------------------------------------------------
uniq_layers, kpts_r, decimals = _unique_with_tol(kpts[:, ax_idx], tol)

# which two coordinates to plot
all_idx = [0, 1, 2]
all_idx.remove(ax_idx)
xidx, yidx = all_idx[0], all_idx[1]
axis_names = {0: 'x', 1: 'y', 2: 'z'}
proj_label = f"{axis_names[xidx]}{axis_names[yidx]}"

nlay = len(uniq_layers)
nrows = int(math.ceil(nlay / ncols))
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
nrows = int(math.ceil(nlay / ncols))
nrows = int(np.ceil(nlay / ncols))

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
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 uses AI. Check for mistakes.

self.fig, axes = plt.subplots(nrows, ncols, figsize=(5.5 * ncols, 5.5 * nrows))
axes = np.atleast_1d(axes).ravel()
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
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 uses AI. Check for mistakes.

# Color normalization across all layers (so colors mean the same everywhere)
# Matplotlib will auto-scale by default; this keeps it global if you pass vmin/vmax.
if ('vmin' not in kwargs) and ('vmax' not in kwargs):
kwargs = dict(kwargs) # avoid mutating caller dict
kwargs['vmin'] = np.nanmin(data)
kwargs['vmax'] = np.nanmax(data)

lim = 1.05 * np.linalg.norm(self.rlat[0])

last_plot = None
for i, layer_val in enumerate(uniq_layers):
ax = axes[i]

# BZ border (same as original)
ax.add_patch(BZ_Wigner_Seitz(self.lattice))

ax.set_xlim(-lim, lim)
ax.set_ylim(-lim, lim)
ax.set_aspect('equal')

ax.set_title(f"{axis} ≈ {layer_val:g} (rounded {decimals} dp)")

# select points in this layer
mask = (kpts_r == layer_val)
Copy link

Copilot AI Feb 15, 2026

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.

Suggested change
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 uses AI. Check for mistakes.
k_layer = kpts[mask]
d_layer = np.asarray(data)[mask]

# reproduce plot also in adjacent BZs (needs 2D shifts on the projected plane)
BZs = shifted_grids_2D(k_layer[:, [xidx, yidx]], self.rlat)

# shifted_grids_2D expects 2D kpts; we use only the projected coords
for kpts_s in BZs:
last_plot = ax.scatter(kpts_s[:, 0], kpts_s[:, 1], c=d_layer, **kwargs)

ax.set_xlabel(axis_names[xidx])
ax.set_ylabel(axis_names[yidx])

# Turn off unused axes
for j in range(nlay, len(axes)):
axes[j].axis('off')

# One shared colorbar for the whole figure
if plt_cbar and last_plot is not None:
self.fig.colorbar(last_plot, ax=axes[:nlay], shrink=0.9)

self.fig.suptitle(f"3D slices: projection on {proj_label}-plane, grouped by {axis}", y=0.995)

self.fig.tight_layout()

if plt_show:
plt.show()
else:
print("3D subplot grid ready.\nYou can customise adding savefig, title, labels, text, show, etc...")
Copy link

Copilot AI Feb 15, 2026

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.

Copilot uses AI. Check for mistakes.

def __str__(self,verbose=False):

Expand Down
Loading