dis_plot can plot a single point by particle and also a cut #1119
Conversation
📝 WalkthroughWalkthroughDistribution plotting logic was split between 1D and 2D based on Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant dist_plot
participant HistEngine as Histogram Engine
participant Filter as Filter (Gaussian/Median)
participant Renderer as Matplotlib Renderer
Caller->>dist_plot: call with axis, vbins, weights, cuts, options
dist_plot->>dist_plot: evaluate len(axis)
alt 2D path
dist_plot->>HistEngine: compute 2D histogram (histogram2d) with weights
HistEngine-->>dist_plot: hist, xedges, yedges
alt filtering requested
dist_plot->>Filter: apply gaussian or median filter
Filter-->>dist_plot: filtered_hist
end
dist_plot->>Renderer: render image (colormap + colorbar) or plain markers/lines
Renderer-->>Caller: display/return figure
else 1D path
dist_plot->>dist_plot: apply cuts -> new_particles (optional)
dist_plot->>HistEngine: compute 1D histogram from (new_)particles
HistEngine-->>dist_plot: hist, edges
dist_plot->>Renderer: plot line (drawstyle) and averages/bulk lines
Renderer-->>Caller: display/return figure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| stride = kwargs.get("stride", 1) | ||
| markersize = kwargs.get("markersize", 0.5) | ||
| alpha = kwargs.get("alpha", 0.5) | ||
| im = ax.plot(x[::stride], y[::stride], |
Check notice
Code scanning / CodeQL
Unused local variable Note
There was a problem hiding this comment.
yep, like the one defined with the previous pcolormesh. But lets keep this handle around !
There was a problem hiding this comment.
yep, but let's keep it,
| cmin = kwargs.get("color_min", h.min()) | ||
| cmin = max(cmin, 1e-4) | ||
| xh_ = 0.5*(xh[:-1]+xh[1:]) | ||
| im = ax.plot(xh_, h, drawstyle=drawstyle) |
Check notice
Code scanning / CodeQL
Unused local variable Note
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pyphare/pyphare/pharesee/plotting.py (3)
167-184:new_particlesundefined in 2D axis mode, causingNameErrorwhenbulk=True.The variable
new_particlesis only assigned in thelen(axis) == 1branch (lines 124-136). Whenlen(axis) == 2andbulk=True, this code will raiseNameError.Additionally, the reference to
axis[1]at line 177 will causeIndexErrorin 1D mode.🐛 Proposed fix
if "bulk" in kwargs: if kwargs["bulk"] is True: + # Use particles for 2D, new_particles for 1D + bulk_particles = new_particles if len(axis) == 1 else particles if axis[0] in vaxis: ax.axvline( np.average( - new_particles.v[:, vaxis[axis[0]]], weights=new_particles.weights + bulk_particles.v[:, vaxis[axis[0]]], weights=bulk_particles.weights ), color="w", ls="--", ) - if axis[1] in vaxis: + if len(axis) > 1 and axis[1] in vaxis: ax.axhline( np.average( - new_particles.v[:, vaxis[axis[1]]], weights=new_particles.weights + bulk_particles.v[:, vaxis[axis[1]]], weights=bulk_particles.weights ), color="w", ls="--", )
189-197:interpfeature uses variables only defined in 2D mode.The interpolation logic references
yhandimage, which are only defined whenlen(axis) == 2. Ifinterp=Truewith a 1D axis specification, this will raiseNameError.🐛 Proposed fix: guard interp block for 2D only
interp = kwargs.get("interp", False) - if interp: + if interp and len(axis) == 2: xbins = 0.5 * (xh[1:] + xh[:-1]) ybins = 0.5 * (yh[1:] + yh[:-1]) xx, yy = np.meshgrid(xbins, ybins, indexing="ij") coords = np.array([xx.flatten(), yy.flatten()]).T interpdist = LinearNDInterpolator(coords, image.T.flatten()) return fig, ax, interpdist, xbins, ybins + elif interp and len(axis) == 1: + raise ValueError("interp=True is not supported for 1D axis plots")
154-157: KDE feature usesywhich is undefined in 1D mode.The
kdeblock references bothxandy, but in the 1D axis branch,yis never defined. This will causeNameErrorwhenkde=Truewith a single-axis specification.🐛 Proposed fix: guard KDE for 2D only
- if kwargs.get("kde", False) is True: + if kwargs.get("kde", False) is True and len(axis) == 2: import seaborn as sns sns.kdeplot(x=x, y=y, ax=ax, color="w")
🤖 Fix all issues with AI agents
In @pyphare/pyphare/pharesee/plotting.py:
- Around line 95-101: The code sets norm only for "log" and "linear" color_scale
values, so if color_scale is anything else norm remains uninitialized and
ax.pcolormesh will raise UnboundLocalError; fix by adding an explicit else
branch after the existing if/elif to either (a) raise a clear ValueError
mentioning the unsupported color_scale, or (b) fall back to a default (e.g., set
norm = Normalize(cmin, cmax)); update the block around
color_scale/LogNorm/Normalize and the ax.pcolormesh call so norm is always
defined or an exception is raised with a helpful message.
- Around line 56-67: The code in the block handling len(axis)==2 can leave x or
y undefined (used later in np.histogram2d) when axis values aren't matched;
ensure x and y are always assigned before the histogram call by handling all
expected axis name cases (e.g., check axis[0]/axis[1] for vaxis entries and for
explicit spatial names like "x","y","z"), and if an unknown axis is passed raise
a clear ValueError; update the branch logic around vaxis, axis, and particles
(the symbols vaxis, axis, particles, and the histogram2d call) so x and y are
initialized for every valid input path or an explicit error is thrown.
🧹 Nitpick comments (1)
pyphare/pyphare/pharesee/plotting.py (1)
387-388: LaTeX string uses\\which renders as a line break, not a space.The double backslash in LaTeX (
$x \\ (d_p)$) creates a line break. For a space betweenxand(d_p), use\(backslash-space) or a regular space.♻️ Suggested fix
- ax.set_xlabel(kwargs.get("xlabel", "$x \\ (d_p)$")) - ax.set_ylabel(kwargs.get("ylabel", "$y \\ (d_p)$")) + ax.set_xlabel(kwargs.get("xlabel", r"$x\ (d_p)$")) + ax.set_ylabel(kwargs.get("ylabel", r"$y\ (d_p)$"))Using raw strings (
r"...") avoids escaping issues with backslashes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy.pypyphare/pyphare/pharesee/plotting.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T13:23:32.074Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
Applied to files:
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
🧬 Code graph analysis (1)
pyphare/pyphare/pharesee/plotting.py (2)
pyphare/pyphare/pharesee/particles.py (4)
x(46-49)y(52-55)size(75-76)select(102-135)pyphare/pyphare/core/box.py (1)
Box(5-69)
🪛 GitHub Check: CodeQL
pyphare/pyphare/pharesee/plotting.py
[failure] 66-66: Potentially uninitialized local variable
Local variable 'x' may be used before it is initialized.
[failure] 66-66: Potentially uninitialized local variable
Local variable 'y' may be used before it is initialized.
[failure] 101-101: Potentially uninitialized local variable
Local variable 'norm' may be used before it is initialized.
[failure] 145-145: Potentially uninitialized local variable
Local variable 'x' may be used before it is initialized.
[notice] 109-109: Unused local variable
Variable im is not used.
[notice] 149-149: Unused local variable
Variable im is not used.
🪛 Ruff (0.14.10)
pyphare/pyphare/pharesee/plotting.py
82-84: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
586-594: LGTM!The axis length branching logic correctly handles both 2D and 1D axis specifications. The 2D case preserves the original behavior, and the 1D case appropriately assigns
vbinsdirectly without attempting to accessaxis[0].
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pyphare/pyphare/pharesee/plotting.py (3)
162-165:NameErrorwhenkde=Truewith 1D axis.In the 1D branch (
len(axis) == 1), onlyxis defined. The KDE section referencesywhich will raiseNameError. KDE plotting should be guarded to only run for 2D cases.🐛 Proposed fix
- if kwargs.get("kde", False) is True: + if kwargs.get("kde", False) is True and len(axis) == 2: import seaborn as sns sns.kdeplot(x=x, y=y, ax=ax, color="w")
175-192:NameErrorandIndexErrorin bulk velocity section.Two issues here:
new_particlesundefined in 2D branch:new_particlesis only assigned in the 1D branch (lines 134-142). When usingbulk=Truewith a 2D axis, lines 180 and 188 will raiseNameError.
IndexErroronaxis[1]in 1D case: Line 185 accessesaxis[1], but whenlen(axis) == 1, this will raiseIndexError.🐛 Proposed fix
if "bulk" in kwargs: if kwargs["bulk"] is True: + # Use appropriate particles reference based on branch + bulk_particles = new_particles if len(axis) == 1 else particles if axis[0] in vaxis: ax.axvline( np.average( - new_particles.v[:, vaxis[axis[0]]], weights=new_particles.weights + bulk_particles.v[:, vaxis[axis[0]]], weights=bulk_particles.weights ), color="w", ls="--", ) - if axis[1] in vaxis: + if len(axis) == 2 and axis[1] in vaxis: ax.axhline( np.average( - new_particles.v[:, vaxis[axis[1]]], weights=new_particles.weights + bulk_particles.v[:, vaxis[axis[1]]], weights=bulk_particles.weights ), color="w", ls="--", )
197-205:NameErrorwheninterp=Truewith 1D axis.Variables
yhandimageare only defined in the 2D branch. Usinginterp=Truewith a 1D axis will raiseNameError. The interpolation feature is inherently 2D, so it should be guarded.🐛 Proposed fix
interp = kwargs.get("interp", False) - if interp: + if interp and len(axis) == 2: xbins = 0.5 * (xh[1:] + xh[:-1]) ybins = 0.5 * (yh[1:] + yh[:-1]) xx, yy = np.meshgrid(xbins, ybins, indexing="ij") coords = np.array([xx.flatten(), yy.flatten()]).T interpdist = LinearNDInterpolator(coords, image.T.flatten()) return fig, ax, interpdist, xbins, ybins + elif interp and len(axis) == 1: + raise ValueError("interp=True is only supported for 2D axis")
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/pharesee/plotting.py`:
- Around line 130-140: The cuts are meant to be in position space but
particles.select(box_new) defaults to cell indices; update the ndim==1 branch in
plotting.py to call particles.select(box_new, box_type="pos") so the Box bounds
are interpreted as positions (use symbols cuts, Box, particles.select, ndim); do
not change the 2D/3D branches until particles.select supports box_type="pos" for
ndim>1 (or add an explicit guard/clear TODO comment/assert there).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy.pypyphare/pyphare/pharesee/plotting.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py
🧰 Additional context used
🧬 Code graph analysis (1)
pyphare/pyphare/pharesee/plotting.py (2)
pyphare/pyphare/pharesee/particles.py (3)
x(46-49)y(52-55)select(102-135)pyphare/pyphare/core/box.py (1)
Box(5-69)
🪛 Ruff (0.14.11)
pyphare/pyphare/pharesee/plotting.py
62-62: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
86-88: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (3)
pyphare/pyphare/pharesee/plotting.py (3)
56-71: LGTM for axis extraction and error handling.The uninitialized variable issues flagged in previous reviews are now properly addressed with explicit
ValueErrorraises for unsupported axis combinations.
73-88: Well-structured filter handling with proper mutual exclusion.The logic correctly ensures gaussian and median filters cannot be applied simultaneously, with a clear error message when both are specified.
395-396: Good improvement to default axis labels with units.The updated default labels with proper LaTeX formatting and units (
$d_p$) improve clarity for users.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pyphare/pyphare/pharesee/plotting.py (3)
162-165:NameErrorwhenkde=Truewith 1D axis.The KDE plot uses both
xandy, butyis only defined in the 2D branch. Usingkde=Truewithlen(axis) == 1will crash.🐛 Proposed fix to guard KDE for 2D only
- if kwargs.get("kde", False) is True: + if kwargs.get("kde", False) is True and len(axis) == 2: import seaborn as sns sns.kdeplot(x=x, y=y, ax=ax, color="w")
175-192:NameErrorwhenbulk=Truewith 2D axis.
new_particlesis only assigned in the 1D branch (insidelen(axis) == 1). For 2D plots, this code path will crash becausenew_particlesis undefined.🐛 Proposed fix to use correct particle reference
if "bulk" in kwargs: if kwargs["bulk"] is True: + # Use new_particles if defined (1D with cuts), otherwise use particles + p = new_particles if 'new_particles' in dir() else particles if axis[0] in vaxis: ax.axvline( np.average( - new_particles.v[:, vaxis[axis[0]]], weights=new_particles.weights + p.v[:, vaxis[axis[0]]], weights=p.weights ), color="w", ls="--", ) - if axis[1] in vaxis: + if len(axis) == 2 and axis[1] in vaxis: ax.axhline( np.average( - new_particles.v[:, vaxis[axis[1]]], weights=new_particles.weights + p.v[:, vaxis[axis[1]]], weights=p.weights ), color="w", ls="--", )Alternatively, define
new_particles = particlesat the start ofdist_plotand only reassign when cuts are applied.
197-205:NameErrorwheninterp=Truewith 1D axis.Variables
yhandimageare only defined in the 2D branch. The interpolation feature will crash for 1D plots.🐛 Proposed fix to guard interp for 2D only
interp = kwargs.get("interp", False) - if interp: + if interp and len(axis) == 2: xbins = 0.5 * (xh[1:] + xh[:-1]) ybins = 0.5 * (yh[1:] + yh[:-1]) xx, yy = np.meshgrid(xbins, ybins, indexing="ij") coords = np.array([xx.flatten(), yy.flatten()]).T interpdist = LinearNDInterpolator(coords, image.T.flatten()) return fig, ax, interpdist, xbins, ybins + elif interp and len(axis) == 1: + raise ValueError("interp=True is only supported for 2D distribution plots")
🧹 Nitpick comments (1)
pyphare/pyphare/pharesee/plotting.py (1)
126-126: Dead code:binsis reassigned before use.This assignment is immediately overwritten at line 151 before
binsis ever used. Remove this line to avoid confusion.♻️ Suggested fix
elif len(axis) == 1: - bins = kwargs.get("bins", (50)) cuts = kwargs.get("cuts", None) ndim = particles.ndim
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy.pypyphare/pyphare/pharesee/plotting.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py
🧰 Additional context used
🧬 Code graph analysis (1)
pyphare/pyphare/pharesee/plotting.py (2)
pyphare/pyphare/pharesee/particles.py (3)
x(46-49)y(52-55)select(102-135)pyphare/pyphare/core/box.py (1)
Box(5-69)
🪛 Ruff (0.14.11)
pyphare/pyphare/pharesee/plotting.py
62-62: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
86-88: Avoid specifying long messages outside the exception class
(TRY003)
105-105: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
pyphare/pyphare/pharesee/plotting.py (2)
56-71: LGTM - Axis handling properly guards against uninitialized variables.The ValueError raises for unsupported axis combinations correctly address the static analysis warnings from previous reviews. The explicit error messages make the supported axis combinations clear.
395-396: LGTM - Axis labels updated to use ion inertial length units.The label change from
$x c / \omega_p$to$x \ (d_p)$provides cleaner notation using the ion inertial length normalization.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| elif ndim == 2: | ||
| box_new = Box((cuts[0][0], cuts[1][0]), (cuts[0][1], cuts[1][1])) # TODO need to be tested | ||
| new_particles = particles.select(box_new, box_type="pos") | ||
| else: | ||
| box_new = Box((cuts[0][0], cuts[1][0], cuts[2][0]), (cuts[0][1], cuts[1][1], cuts[2][1])) # TODO need to be tested | ||
| new_particles = particles.select(box_new, box_type="pos") |
There was a problem hiding this comment.
box_type="pos" will fail for ndim > 1.
The particles.select() method asserts self.ndim == 1 when box_type="pos" is used. The 2D and 3D branches here will raise AssertionError at runtime.
Since these branches are marked TODO, consider adding explicit guards or raising NotImplementedError until select() is extended.
🐛 Proposed fix to prevent runtime assertion failures
if cuts is not None:
from pyphare.core.box import Box
if ndim == 1:
box_new = Box(cuts[0][0], cuts[0][1])
new_particles = particles.select(box_new, box_type="pos")
elif ndim == 2:
- box_new = Box((cuts[0][0], cuts[1][0]), (cuts[0][1], cuts[1][1])) # TODO need to be tested
- new_particles = particles.select(box_new, box_type="pos")
+ raise NotImplementedError("Position-space cuts for 2D simulations not yet supported")
else:
- box_new = Box((cuts[0][0], cuts[1][0], cuts[2][0]), (cuts[0][1], cuts[1][1], cuts[2][1])) # TODO need to be tested
- new_particles = particles.select(box_new, box_type="pos")
+ raise NotImplementedError("Position-space cuts for 3D simulations not yet supported")1 participant
To plot a
dis_plotusing a point per particle (and not apcolormeshof a given binning of the phase space), the command line could bewhere
plainsays we want a point per particle,stridesays that we plot 1 overstrideparticle,color,alphaandmarkersizebeing straightforward.It could also be
to have a 1-D distribution (that is
f(v_x)in this example). By default,drawstyle="mid-steps"to make sure that the choice for the binning is explicit, so thatdrawstyle="default"can be a kwargs. Hence,cutsis a tuple of dimndim, where each element is a tuple of 2 elements containg lower and upper values of the cutting box for the iven direction. By default,cuts=None, meaning that all the particles are considered.It has to be clear that even if the kwarg name is
cut, this is a cut in position space, but not in velocity space. It means that in the plot off(v_x)for example, we have an implicit summation overv_yandv_z.