add a gaussian_filter and find_peaks for scalarfield and vectorfield #1120
Conversation
📝 WalkthroughWalkthroughAdds Gaussian-filter and peak-detection utilities for hierarchies, extends Yee-centering mappings to include Changes
Sequence Diagram(s)mermaid mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2024-10-18T13:23:32.074ZApplied to files:
📚 Learning: 2024-09-09T13:57:02.285ZApplied to files:
🧬 Code graph analysis (3)pyphare/pyphare/pharesee/run/utils.py (4)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
pyphare/pyphare/core/ufuncs.py (4)
🪛 Ruff (0.14.11)pyphare/pyphare/pharesee/run/utils.py532-532: Avoid specifying long messages outside the exception class (TRY003) 535-535: Avoid specifying long messages outside the exception class (TRY003) pyphare/pyphare/pharesee/hierarchy/hierarchy.py622-622: Prefer Replace with (RUF015) pyphare/pyphare/core/ufuncs.py72-72: Prefer Replace with (RUF015) 120-120: Avoid specifying long messages outside the exception class (TRY003) 136-136: Avoid specifying long messages outside the exception class (TRY003) 147-147: Avoid specifying long messages outside the exception class (TRY003) 155-155: Add explicit value for parameter (B905) ⏰ 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)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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}
|
|
||
| return final, dp(final, **kwargs) | ||
|
|
||
| def interpol(self, time, interp="nearest"): |
Check notice
Code scanning / CodeQL
Cyclic import 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: 6
🤖 Fix all issues with AI agents
In @pyphare/pyphare/core/ufuncs.py:
- Around line 136-138: Replace the incorrect return of the sentinel
NotImplemented with raising a proper exception: in the function that assigns
time = times[0] and currently does "return NotImplemented", change that to raise
NotImplementedError with a short explanatory message (similar to the gFilt fix)
so callers get an exception instead of a bogus return value.
- Around line 81-89: The branch that currently returns NotImplemented should
instead raise NotImplementedError and the function must explicitly handle the
case hier.ndim != 1; update the block in ufuncs.py where hier is tested for ndim
== 1 (and where ScalarField/VectorField are handled via compute_hier_from and
_compute_gaussian_filter_on_scalarfield/_compute_gaussian_filter_on_vectorfield
with nb_ghosts and sigma) so that if hier is neither ScalarField nor VectorField
you raise NotImplementedError("Gaussian filter not implemented for type ..."),
and if hier.ndim != 1 raise NotImplementedError(f"Gaussian filter only
implemented for 1D hier, got ndim={hier.ndim}").
- Around line 173-175: The loop assumes pks[1]['peak_heights'] exists and uses
zip(pks[0], pks[1]['peak_heights']); update the code in the block that builds
pks_ and hgs_ to (1) guard the access to the 'peak_heights' key on pks[1] (e.g.,
check if 'peak_heights' in pks[1] or fetch via .get and handle a missing value
by skipping, defaulting, or raising a clear ValueError) and (2) call zip(...,
strict=True) so the zip between pks[0] and the heights enforces equal lengths;
keep references to pks, pks_[the accumulator], hgs_, patch.dl and patch.origin
when implementing the fix.
- Around line 115-123: In the ndim dispatch in pyphare/core/ufuncs.py replace
the duplicated "elif ndim == 2" branch with "elif ndim == 3" so the 3D call uses
func(r_[0], r_[1], r_[2]); also change the final "print('unvalid dimension')" to
raise a ValueError with a clear message (e.g., "invalid dimension") so invalid
ndim values are handled as exceptions; references: variables ndim, func, r_ in
the existing branch logic.
In @pyphare/pyphare/pharesee/hierarchy/scalarfield.py:
- Line 3: Remove the unused import flat_finest_field from
pyphare.pharesee.hierarchy.scalarfield by deleting the line that imports
flat_finest_field; search for the symbol flat_finest_field in this file to
confirm it's unused and ensure no other references exist before removing the
import statement from the top of the module.
In @pyphare/pyphare/pharesee/run/utils.py:
- Line 508: The nCells computation is off-by-one: replace the manual formula
nCells = box.upper - box.lower + 2 with the correct count using either nCells =
box.upper - box.lower + 1 or, better, use the Box.shape property (e.g., nCells =
box.shape) so the interpolation uses the correct grid dimensions; update the
occurrence of nCells in this module (the variable computed from box.upper and
box.lower) to use box.shape to avoid the extra cell.
🧹 Nitpick comments (8)
pyphare/pyphare/core/operators.py (1)
22-29: LGTM! Consider removing the TODO comment.The refactoring to use
patch_datas["value"].centeringsdirectly is clearer and removes unnecessary indirection. Since the TODO has been addressed by this change, consider removing the comment.♻️ Optional: Remove the TODO comment
def _compute_sqrt(patch_datas, **kwargs): - # ref_name = next(iter(patch_datas.keys())) TODO this is always "value" - dset = np.sqrt(patch_datas["value"][:]) return (pyphare/pyphare/pharesee/run/utils.py (1)
493-537: Significant code duplication withmake_interpolator.The
build_interpolatorfunction (lines 493-537) duplicates substantial logic frommake_interpolator(lines 444-490). Both functions:
- Create scipy interpolators (interp1d, LinearNDInterpolator, NearestNDInterpolator)
- Generate finest coordinates using yeeCoordsFor
- Handle 1D and 2D cases with similar error handling
Consider refactoring to extract common logic and reduce duplication.
💡 Refactoring suggestion
Consider creating a shared helper function for the interpolator creation logic, with parameters to handle the differences in coordinate generation (
domainvsboxbased).pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
619-631: Consider defensive checks and usenext()for cleaner iteration.The
interpolmethod has several assumptions that could cause runtime errors:
- Line 623 uses
list(...)[0]whennext(iter(...))would be more idiomatic and efficient.- The method assumes level 0 exists and has at least one patch with patch_datas.
- It assumes
nbrGhostsis consistent across all quantities.- Lines 627-628 assume each quantity exists in the first patch's patch_datas.
Consider adding validation checks or documenting these assumptions.
♻️ Proposed improvements
def interpol(self, time, interp="nearest"): from pyphare.pharesee.hierarchy.hierarchy_utils import flat_finest_field from pyphare.pharesee.run.utils import build_interpolator - nbrGhosts = list(self.level(0).patches[0].patch_datas.values())[0].ghosts_nbr + # Get nbrGhosts from the first available patch data + first_patch = self.level(0).patches[0] + nbrGhosts = next(iter(first_patch.patch_datas.values())).ghosts_nbr interp_ = {} for qty in self.quantities(): box = self.level(0).patches[0].patch_datas[qty].box dl = self.level(0).patches[0].patch_datas[qty].dl data, coords = flat_finest_field(self, qty, time=time) interp_[qty] = build_interpolator(data, coords, interp, box, dl, qty, nbrGhosts) return interp_pyphare/pyphare/core/ufuncs.py (5)
5-8: Remove unused imports.These imports are not used anywhere in the module and should be removed.
Suggested fix
-from pyphare.pharesee.hierarchy.hierarchy_utils import are_compatible_hierarchies -from pyphare.core.phare_utilities import listify -from pyphare.pharesee.hierarchy.patch import Patch -from pyphare.pharesee.hierarchy.patchlevel import PatchLevel
14-16: Remove redundant import.
gaussian_filteris already imported at module level (line 10). The local import is unnecessary.Suggested fix
def _compute_gaussian_filter_on_scalarfield(patch_datas, **kwargs): - from scipy.ndimage import gaussian_filter - ndim = patch_datas["value"].box.ndim
33-35: Remove redundant import.Same issue as above —
gaussian_filteris already available from the module-level import.Suggested fix
def _compute_gaussian_filter_on_vectorfield(patch_datas, **kwargs): - from scipy.ndimage import gaussian_filter - ref_name = next(iter(patch_datas.keys()))
69-76: Clean up commented-out code and usenext()idiom.The commented-out block appears to be dead code. Line 76 should use
next(iter(...))instead oflist(...)[0]for efficiency.Suggested fix
- # time0 = list(hier.times())[0] - # level0 = 0 - # p0 = 0 - # pd0 = hier.levels(time0)[level0].patches[p0].patch_datas - # key0 = list(pd0.keys())[0] - # nb_ghosts = np.max(pd0[key0].ghosts_nbr) - - nb_ghosts = np.max(list(hier.level(0).patches[0].patch_datas.values())[0].ghosts_nbr) + nb_ghosts = np.max(next(iter(hier.level(0).patches[0].patch_datas.values())).ghosts_nbr)
151-179: Remove commented-out debug code.The extensive commented-out print statements are debug artifacts and clutter the code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pyphare/pyphare/core/gridlayout.pypyphare/pyphare/core/operators.pypyphare/pyphare/core/ufuncs.pypyphare/pyphare/pharesee/hierarchy/hierarchy.pypyphare/pyphare/pharesee/hierarchy/scalarfield.pypyphare/pyphare/pharesee/run/utils.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/core/ufuncs.pypyphare/pyphare/pharesee/hierarchy/scalarfield.pypyphare/pyphare/pharesee/hierarchy/hierarchy.py
🧬 Code graph analysis (4)
pyphare/pyphare/core/ufuncs.py (3)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (2)
compute_hier_from(137-160)are_compatible_hierarchies(73-84)pyphare/pyphare/core/phare_utilities.py (1)
listify(46-51)pyphare/pyphare/pharesee/hierarchy/patch.py (1)
Patch(4-69)
pyphare/pyphare/pharesee/hierarchy/scalarfield.py (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
flat_finest_field(330-350)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (3)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
flat_finest_field(330-350)pyphare/pyphare/pharesee/run/utils.py (1)
build_interpolator(493-537)pyphare/pyphare/core/gridlayout.py (1)
nbrGhosts(247-251)
pyphare/pyphare/pharesee/run/utils.py (1)
pyphare/pyphare/core/gridlayout.py (3)
nbrGhosts(247-251)yeeCoordsFor(150-177)yeeCoordsFor(387-416)
🪛 GitHub Check: CodeQL
pyphare/pyphare/core/ufuncs.py
[warning] 119-119: Redundant comparison
Test is always false, because of this condition.
[warning] 120-120: Unreachable code
This statement is unreachable.
[notice] 5-5: Unused import
Import of 'are_compatible_hierarchies' is not used.
[notice] 6-6: Unused import
Import of 'listify' is not used.
[notice] 7-7: Unused import
Import of 'Patch' is not used.
[notice] 8-8: Unused import
Import of 'PatchLevel' is not used.
[notice] 66-66: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
[notice] 176-179: Commented-out code
This comment appears to contain commented-out code.
pyphare/pyphare/pharesee/hierarchy/scalarfield.py
[notice] 3-3: Unused import
Import of 'flat_finest_field' is not used.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 619-619: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
🪛 Ruff (0.14.10)
pyphare/pyphare/core/ufuncs.py
76-76: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
173-173: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
623-623: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
pyphare/pyphare/pharesee/run/utils.py
532-532: Avoid specifying long messages outside the exception class
(TRY003)
535-535: 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 (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
🔇 Additional comments (4)
pyphare/pyphare/core/gridlayout.py (1)
47-50: LGTM!The expansion of Yee centering mappings to include "value", "x", "y", and "z" keys with "primal" centering is consistent across all three spatial directions and aligns with the PR's goal of supporting Gaussian filtering and peak finding on scalar and vector fields.
Also applies to: 86-89, 125-128
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
442-444: LGTM!The addition of the
lwparameter retrieval improves the plot1d method's flexibility.
619-619: Cyclic import is acceptable in this context.The cyclic import warning can be safely ignored. Based on learnings, importing from
hierarchy_utilsintohierarchy.pyis acceptable as long ashierarchy.pydoes not create a circular dependency, which is the established pattern in this module.pyphare/pyphare/core/ufuncs.py (1)
125-126: Verify slice bounds whennb_ghosts[i] >= sigma.If
nb_ghosts[i] == sigma, the slice becomesslice(0, 0)which is empty. Ifnb_ghosts[i] > sigma, the slice indices invert (e.g.,slice(2, -2)when sigma=3 and nb_ghosts=5 would select less data than expected). Ensure this is the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @pyphare/pyphare/core/ufuncs.py:
- Around line 173-175: The loop assumes pks[1]['peak_heights'] exists causing a
KeyError when find_peaks was called without height; to fix, before the loop
obtain heights = pks[1].get('peak_heights') and if None compute heights from the
underlying signal at the peak indices (e.g., heights = signal_array[pks[0]]),
then iterate for pk, hg in zip(pks[0], heights) and append to pks_ and hgs_;
alternatively, if computing heights is impossible here, raise a clear error
indicating that the height argument is required.
- Around line 81-89: The branch only handles hier.ndim == 1 and returns the
sentinel NotImplemented for other cases; replace that with explicit error
handling and proper exception raising: instead of returning NotImplemented,
raise NotImplementedError with a message including hier.ndim and the field type,
and extend the conditional to explicitly handle expected dims (e.g., ndim == 2
and ndim == 3) by calling the appropriate compute_hier_from variant or, if those
variants are not available, raise NotImplementedError mentioning
_compute_gaussian_filter_on_scalarfield/_compute_gaussian_filter_on_vectorfield
and the unsupported dimension; ensure ScalarField/VectorField handling remains
intact and that compute_hier_from is invoked with nb_ghosts and sigma as before.
- Around line 115-123: The branch contains a copy-paste bug: change the second
`elif ndim == 2` to `elif ndim == 3` and call `func(r_[0], r_[1], r_[2])` to
produce `data` for 3D; also stop silently printing on invalid dimensions—raise a
ValueError (or return/raise) instead of `print('unvalid dimension')` so `data`
is always defined before subsequent use (e.g., the later `gaussian_filter`
call).
- Around line 143-145: The code mutates the caller's kwargs by calling
kwargs.pop("names"); instead, read names with kwargs.get("names") into names_
(or make a shallow copy like local_kwargs = dict(kwargs) and pop from that) and
then pass a filtered/local_kwargs to find_peaks so the original kwargs is not
modified; update the block that sets names_ and the subsequent call to
find_peaks to use the non-mutating approach (symbols to change: names_, kwargs,
find_peaks).
- Around line 5-10: Remove the unused imports at the top of ufuncs.py
(are_compatible_hierarchies, listify, Patch, PatchLevel) since they are not
referenced, and keep a single top-level "from scipy.ndimage import
gaussian_filter" import; also delete the redundant inner imports of
gaussian_filter inside the functions _compute_gaussian_filter_on_scalarfield and
_compute_gaussian_filter_on_vectorfield so both functions use the single
module-level gaussian_filter.
In @pyphare/pyphare/pharesee/hierarchy/hierarchy.py:
- Around line 618-630: The interpol method uses the provided time only for
flat_finest_field but calls self.level(0) without the time argument for
nbrGhosts, box and dl, and uses list(...)[0] to get the first patch; fix by
calling self.level(0, time=time) (or the correct API to request the same
timestamp) whenever accessing patch data for nbrGhosts, box and dl so those
values match the data passed to flat_finest_field, and replace
list(self.level(0).patches)[0] / list(...)[0] uses with next(iter(...)) to
satisfy the RUF015 suggestion; update references in interpol, specifically
nbrGhosts, box, dl and the self.level calls, and keep build_interpolator and
flat_finest_field usage unchanged.
In @pyphare/pyphare/pharesee/hierarchy/scalarfield.py:
- Line 3: Remove the unused import flat_finest_field from the top of the file
(the import statement "from pyphare.pharesee.hierarchy.hierarchy_utils import
flat_finest_field" in scalarfield.py); ensure no other references to
flat_finest_field remain in this module and run linters/tests to confirm the
unused-import warning is resolved.
In @pyphare/pyphare/pharesee/run/utils.py:
- Line 535: Update the ValueError message to reference the correct function
name: replace the string "make_interpolator is not yet 3d" with
"build_interpolator is not yet 3d" in the raise statement (the one that
currently raises ValueError near the build_interpolator logic) so the error
message matches the actual function name.
- Line 508: The nCells calculation is off by one: change the expression that
sets nCells from using "box.upper - box.lower + 2" to use "box.upper - box.lower
+ 1" (or simply use "box.shape") so it matches Box semantics and aligns with
yeeCoordsFor's internal +1 for primal centering; update the assignment where
nCells is computed in utils.py (the variable nCells and the surrounding
calculation) accordingly.
🧹 Nitpick comments (5)
pyphare/pyphare/core/operators.py (1)
22-29: LGTM with minor cleanup suggestion.The simplification to directly use
patch_datas["value"]is correct since_compute_sqrtoperates onScalarFieldwhich always has a"value"key. Consider removing the commented-out line entirely rather than leaving it as a TODO comment.🧹 Suggested cleanup
def _compute_sqrt(patch_datas, **kwargs): - # ref_name = next(iter(patch_datas.keys())) TODO this is always "value" - dset = np.sqrt(patch_datas["value"][:]) return ( {"name": "value", "data": dset, "centering": patch_datas["value"].centerings}, )pyphare/pyphare/core/ufuncs.py (2)
76-76: Usenext()instead of single-element slice.Per Ruff RUF015, prefer
next(iter(...))overlist(...)[0]for accessing the first element.♻️ Proposed fix
- nb_ghosts = np.max(list(hier.level(0).patches[0].patch_datas.values())[0].ghosts_nbr) + nb_ghosts = np.max(next(iter(hier.level(0).patches[0].patch_datas.values())).ghosts_nbr)
151-179: Remove commented-out debug code before merging.Lines 151-163 and 167-179 contain commented-out debug prints. These should be removed to keep the codebase clean.
pyphare/pyphare/pharesee/run/utils.py (1)
493-537: Consider refactoring to reduce duplication withmake_interpolator.
build_interpolatorshares significant logic withmake_interpolator(lines 444-490). The main differences are:
- Coordinate generation:
build_interpolatorusesboxwhilemake_interpolatorusesdomainnCellscalculation differsConsider extracting the common interpolator construction logic into a shared helper to reduce duplication.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
618-620: Consider adding a docstring for the new public method.The
interpolmethod would benefit from documentation explaining its purpose, parameters, and return type. Based onbuild_interpolator's signature, this returns a dict mapping quantity names to(interpolator, finest_coords)tuples, which isn't immediately obvious.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pyphare/pyphare/core/gridlayout.pypyphare/pyphare/core/operators.pypyphare/pyphare/core/ufuncs.pypyphare/pyphare/pharesee/hierarchy/hierarchy.pypyphare/pyphare/pharesee/hierarchy/scalarfield.pypyphare/pyphare/pharesee/run/utils.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/core/ufuncs.pypyphare/pyphare/pharesee/hierarchy/hierarchy.pypyphare/pyphare/pharesee/hierarchy/scalarfield.py
📚 Learning: 2024-09-09T13:57:02.285Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Applied to files:
pyphare/pyphare/core/ufuncs.py
🧬 Code graph analysis (4)
pyphare/pyphare/core/ufuncs.py (3)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (2)
compute_hier_from(137-160)are_compatible_hierarchies(73-84)pyphare/pyphare/core/phare_utilities.py (1)
listify(46-51)pyphare/pyphare/pharesee/hierarchy/patch.py (1)
Patch(4-69)
pyphare/pyphare/pharesee/run/utils.py (1)
pyphare/pyphare/core/gridlayout.py (3)
nbrGhosts(247-251)yeeCoordsFor(150-177)yeeCoordsFor(374-403)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
flat_finest_field(330-350)pyphare/pyphare/pharesee/run/utils.py (1)
build_interpolator(493-537)
pyphare/pyphare/pharesee/hierarchy/scalarfield.py (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
flat_finest_field(330-350)
🪛 GitHub Check: CodeQL
pyphare/pyphare/core/ufuncs.py
[warning] 119-119: Redundant comparison
Test is always false, because of this condition.
[warning] 120-120: Unreachable code
This statement is unreachable.
[notice] 5-5: Unused import
Import of 'are_compatible_hierarchies' is not used.
[notice] 6-6: Unused import
Import of 'listify' is not used.
[notice] 7-7: Unused import
Import of 'Patch' is not used.
[notice] 8-8: Unused import
Import of 'PatchLevel' is not used.
[notice] 66-66: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
[notice] 176-179: Commented-out code
This comment appears to contain commented-out code.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 619-619: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/scalarfield.py
[notice] 3-3: Unused import
Import of 'flat_finest_field' is not used.
🪛 Ruff (0.14.10)
pyphare/pyphare/core/ufuncs.py
76-76: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
173-173: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
pyphare/pyphare/pharesee/run/utils.py
532-532: Avoid specifying long messages outside the exception class
(TRY003)
535-535: Avoid specifying long messages outside the exception class
(TRY003)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
622-622: Prefer next(...) over single element slice
Replace with next(...)
(RUF015)
⏰ 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-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (3)
pyphare/pyphare/core/gridlayout.py (1)
47-50: LGTM!The additions of
"value","x","y", and"z"keys with"primal"centering extend the Yee centering mappings to supportScalarFieldandVectorFieldquantities. This is consistent with how moment-like quantities are centered and enables proper centering lookups in operators and ufuncs.Also applies to: 86-89, 125-128
pyphare/pyphare/core/ufuncs.py (1)
125-126: Verify slice calculation logic.The slice
slice(sigma-nb_ghosts[i], -sigma+nb_ghosts[i])differs from the pattern used in_compute_gaussian_filter_on_scalarfieldwhich usesslice(nb_ghosts, -nb_ghosts). Whensigma < nb_ghosts, this producesslice(negative, positive)which may not behave as expected (e.g.,sigma=2, nb_ghosts=4yieldsslice(-2, 2)).pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
441-443: LGTM!The addition of the
lw(line width) parameter with a sensible default of 1 is a clean enhancement that maintains backward compatibility while providing additional customization.
| for pk, hg in zip(pks[0], pks[1]['peak_heights']): | ||
| pks_ = np.append(pks_, np.add(np.multiply(pk, patch.dl), patch.origin)) | ||
| hgs_ = np.append(hgs_, hg) |
There was a problem hiding this comment.
peak_heights key requires height argument to be passed.
The code assumes pks[1]['peak_heights'] exists, but scipy.signal.find_peaks only includes this key when the height argument is provided. If a user calls peakIds(hier) without height, this will raise a KeyError.
🐛 Proposed fix
- for pk, hg in zip(pks[0], pks[1]['peak_heights']):
+ heights = pks[1].get('peak_heights', [None] * len(pks[0]))
+ for pk, hg in zip(pks[0], heights, strict=True):
pks_ = np.append(pks_, np.add(np.multiply(pk, patch.dl), patch.origin))
hgs_ = np.append(hgs_, hg)Alternatively, document that height is a required argument, or raise a clear error if it's not provided.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for pk, hg in zip(pks[0], pks[1]['peak_heights']): | |
| pks_ = np.append(pks_, np.add(np.multiply(pk, patch.dl), patch.origin)) | |
| hgs_ = np.append(hgs_, hg) | |
| heights = pks[1].get('peak_heights', [None] * len(pks[0])) | |
| for pk, hg in zip(pks[0], heights, strict=True): | |
| pks_ = np.append(pks_, np.add(np.multiply(pk, patch.dl), patch.origin)) | |
| hgs_ = np.append(hgs_, hg) |
🧰 Tools
🪛 Ruff (0.14.10)
173-173: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
In @pyphare/pyphare/core/ufuncs.py around lines 173 - 175, The loop assumes
pks[1]['peak_heights'] exists causing a KeyError when find_peaks was called
without height; to fix, before the loop obtain heights =
pks[1].get('peak_heights') and if None compute heights from the underlying
signal at the peak indices (e.g., heights = signal_array[pks[0]]), then iterate
for pk, hg in zip(pks[0], heights) and append to pks_ and hgs_; alternatively,
if computing heights is impossible here, raise a clear error indicating that the
height argument is required.
There was a problem hiding this comment.
Hmmm... I realize that the output of find_peaks depend on the kwargs... meaning that their height for example are given only if this kwargs is set. We hence need to discuss on how to do it. For now the kwargs peak_heights is mandatory to circumbent this problem.
There was a problem hiding this comment.
@rochSmets, thanks for clarifying! I understand you're making height a required parameter for now.
To prevent users from encountering a confusing KeyError, I'd suggest one of these approaches:
-
Add validation at the start of
peakIds:if "height" not in kwargs: raise ValueError("The 'height' parameter is required for peakIds")
-
Add a docstring documenting the requirement:
def peakIds(hier, **kwargs): """ Find peaks in hierarchy data. Parameters: hier: Hierarchy object **kwargs: Arguments forwarded to scipy.signal.find_peaks. The 'height' parameter is required. """
-
Handle the missing key gracefully:
if 'peak_heights' in pks[1]: for pk, hg in zip(pks[0], pks[1]['peak_heights']): pks_ = np.append(pks_, np.add(np.multiply(pk, patch.dl), patch.origin)) hgs_ = np.append(hgs_, hg) else: # Handle case where height wasn't provided
Option 1 gives the clearest error message to users. Let me know if you'd like me to help implement any of these!
| @@ -1,5 +1,6 @@ | |||
| from .hierarchy import PatchHierarchy | |||
| from .hierarchy_utils import compute_hier_from, compute_rename, rename, _compute_neg | |||
| from pyphare.pharesee.hierarchy.hierarchy_utils import flat_finest_field | |||
There was a problem hiding this comment.
Remove unused import.
The flat_finest_field import is not used anywhere in this file. As flagged by static analysis, this should be removed.
🧹 Proposed fix
from .hierarchy import PatchHierarchy
-from .hierarchy_utils import compute_hier_from, compute_rename, rename, _compute_neg
-from pyphare.pharesee.hierarchy.hierarchy_utils import flat_finest_field
+from .hierarchy_utils import compute_hier_from, compute_rename, rename, _compute_neg🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 3-3: Unused import
Import of 'flat_finest_field' is not used.
🤖 Prompt for AI Agents
In @pyphare/pyphare/pharesee/hierarchy/scalarfield.py at line 3, Remove the
unused import flat_finest_field from the top of the file (the import statement
"from pyphare.pharesee.hierarchy.hierarchy_utils import flat_finest_field" in
scalarfield.py); ensure no other references to flat_finest_field remain in this
module and run linters/tests to confirm the unused-import warning is resolved.
|
|
||
| dim = coords.ndim | ||
|
|
||
| nCells = box.upper-box.lower+2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how nCells is calculated elsewhere in the codebase
rg -n "nCells\s*=" --type py -C 3Repository: PHAREHUB/PHARE
Length of output: 1088
🏁 Script executed:
# Also inspect the specific file and context around line 508
sed -n '500,520p' pyphare/pyphare/pharesee/run/utils.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 723
🏁 Script executed:
# Search for box.upper and box.lower usage to understand the context
rg -n "box\.(upper|lower)" --type py -B2 -A2 | head -100Repository: PHAREHUB/PHARE
Length of output: 7568
🏁 Script executed:
# Get broader context around line 508
sed -n '490,530p' pyphare/pyphare/pharesee/run/utils.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 1600
🏁 Script executed:
# Find the yeeCoordsFor function definition
rg -n "def yeeCoordsFor" --type py -A10Repository: PHAREHUB/PHARE
Length of output: 1768
🏁 Script executed:
# Search for documentation or comments about nCells in relation to primal/dual centering
rg -n "primal|dual|centering" --type py -i -C2 | head -60Repository: PHAREHUB/PHARE
Length of output: 4222
🏁 Script executed:
# Get full yeeCoordsFor function implementation
sed -n '150,200p' pyphare/pyphare/core/gridlayout.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 1952
🏁 Script executed:
# Look for how yeeCoordsFor is used elsewhere with nbrCells parameter
rg -n "yeeCoordsFor" --type py -B2 -A2 | grep -E "(yeeCoordsFor|nCells|nbrCells)" | head -40Repository: PHAREHUB/PHARE
Length of output: 1457
🏁 Script executed:
# Check if there are any tests or examples for build_interpolator
rg -n "build_interpolator" --type py -B3 -A3Repository: PHAREHUB/PHARE
Length of output: 1909
🏁 Script executed:
# Search for Box class definition and nCells method
rg -n "class Box|def nCells" --type py -B1 -A5 | head -80Repository: PHAREHUB/PHARE
Length of output: 2257
🏁 Script executed:
# Look for any comments or documentation around the build_interpolator function
sed -n '493,515p' pyphare/pyphare/pharesee/run/utils.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 944
🏁 Script executed:
# Check what the typical pattern is for cell counts in the codebase
rg -n "upper\s*-\s*lower\s*\+" --type py | head -20Repository: PHAREHUB/PHARE
Length of output: 40
🏁 Script executed:
# Get the full Box class implementation
sed -n '5,78p' pyphare/pyphare/core/box.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 2710
🏁 Script executed:
# Look for tests or usage of build_interpolator
rg -n "build_interpolator" --type py -B5 -A10Repository: PHAREHUB/PHARE
Length of output: 3487
🏁 Script executed:
# Check how shape is computed in Box
rg -n "def shape|@property" pyphare/pyphare/core/box.py -A3Repository: PHAREHUB/PHARE
Length of output: 200
🏁 Script executed:
# Check how yeeCoordsFor is called with nCells in other contexts
rg -n "yeeCoordsFor" pyphare/pyphare/pharesee/run/utils.py -B3 -A3Repository: PHAREHUB/PHARE
Length of output: 1269
🏁 Script executed:
# Look for any tests that use build_interpolator to understand the expected behavior
fd -e py -path "*/test*" | xargs rg -l "build_interpolator" | head -5Repository: PHAREHUB/PHARE
Length of output: 329
🏁 Script executed:
# Check if nCells should match Box.shape or if there's a specific reason for +2
rg -n "upper.*lower.*1|shape.*prod" pyphare/pyphare/core/box.py -B2 -A2Repository: PHAREHUB/PHARE
Length of output: 858
🏁 Script executed:
# Let's check how box.shape relates to the calculation and verify nCells usage
python3 << 'EOF'
# If box.upper=10, box.lower=5:
upper, lower = 10, 5
# Current formula at line 508:
ncells_current = upper - lower + 2
print(f"Current formula (upper - lower + 2): {ncells_current}")
# Box.shape formula:
box_shape = upper - lower + 1
print(f"Box.shape formula (upper - lower + 1): {box_shape}")
# Difference:
print(f"Difference: {ncells_current - box_shape}")
# This means yeeCoordsFor will receive nCells that's 1 larger than box.shape
# Since yeeCoordsFor adds 1 for primal centering, the final size would be:
print(f"\nWith primal centering in yeeCoordsFor:")
print(f"Final size = nCells + 1 = {ncells_current + 1}")
print(f"vs box.shape + 1 = {box_shape + 1}")
EOFRepository: PHAREHUB/PHARE
Length of output: 144
🏁 Script executed:
# Look for tests of build_interpolator or similar interpolation code
rg -n "interpol" pyphare/pyphare/pharesee/hierarchy/hierarchy.py -B5 -A15 | head -50Repository: PHAREHUB/PHARE
Length of output: 1319
🏁 Script executed:
# Search for any comments or issues related to nCells or interpolation
rg -n "nCells|interpolat" pyphare/pyphare/pharesee/run/utils.py -B2 -A2Repository: PHAREHUB/PHARE
Length of output: 2870
🏁 Script executed:
# Let's verify by checking if there are any related bug reports or comments
rg -n "TODO|FIXME|BUG|HACK" pyphare/pyphare/pharesee/run/utils.py -B2 -A2Repository: PHAREHUB/PHARE
Length of output: 40
🏁 Script executed:
# Check the git history for context on this calculation
git log -p --all -S "box.upper-box.lower+2" -- pyphare/pyphare/pharesee/run/utils.py | head -100Repository: PHAREHUB/PHARE
Length of output: 40
🏁 Script executed:
# Check if nCells is truly a scalar or array in build_interpolator context
sed -n '493,540p' pyphare/pyphare/pharesee/run/utils.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 1787
🏁 Script executed:
# Verify Box properties for multi-dimensional cases
rg -n "class.*Box|def __init__" pyphare/pyphare/core/box.py -A8 | head -50Repository: PHAREHUB/PHARE
Length of output: 1408
🏁 Script executed:
# Check test cases to see expected box behavior
rg -n "Box\(" pyphare/pyphare_tests/test_core/test_box.py -B1 -A1 | head -40Repository: PHAREHUB/PHARE
Length of output: 1933
🏁 Script executed:
# Check if nCells is being treated as array and compare with Box.shape behavior
python3 << 'EOF'
import numpy as np
# Simulate Box behavior
lower = np.array([5, 10])
upper = np.array([10, 20])
# Current formula in line 508
nCells_current = upper - lower + 2
print(f"Current formula (upper - lower + 2): {nCells_current}")
# Box.shape formula
box_shape = upper - lower + 1
print(f"Box.shape formula (upper - lower + 1): {box_shape}")
# What yeeCoordsFor receives
print(f"\nEach dimension:")
for i, (curr, shape) in enumerate(zip(nCells_current, box_shape)):
print(f" Dim {i}: current={curr}, shape={shape}, difference={curr - shape}")
EOFRepository: PHAREHUB/PHARE
Length of output: 175
🏁 Script executed:
# Search for any documentation about why +2 is used specifically
rg -n "nCells|box\.upper|box\.lower" pyphare/pyphare/pharesee/run/utils.py -B5 -A1 | grep -A5 -B5 "508"Repository: PHAREHUB/PHARE
Length of output: 404
🏁 Script executed:
# Check if there's any reference to the difference between make_interpolator and build_interpolator
git log --oneline --all -- pyphare/pyphare/pharesee/run/utils.py | head -20Repository: PHAREHUB/PHARE
Length of output: 188
🏁 Script executed:
# Let's check if there are any tests for build_interpolator or the hierarchy.interpol method
find . -name "*.py" -type f -path "*/test*" -exec rg -l "interpol|build_interpolator" {} \;Repository: PHAREHUB/PHARE
Length of output: 650
🏁 Script executed:
# Compare the two similar functions side by side
diff <(sed -n '444,490p' pyphare/pyphare/pharesee/run/utils.py) <(sed -n '493,537p' pyphare/pyphare/pharesee/run/utils.py)Repository: PHAREHUB/PHARE
Length of output: 145
🏁 Script executed:
# Compare the nCells calculations between make_interpolator and build_interpolator
rg -n "nCells\s*=" pyphare/pyphare/pharesee/run/utils.py -B2 -A5Repository: PHAREHUB/PHARE
Length of output: 733
🏁 Script executed:
# Check the interpolator tests to see if they validate the behavior
cat tests/core/numerics/interpolator/interpolator_test.py | head -100Repository: PHAREHUB/PHARE
Length of output: 2790
🏁 Script executed:
# Look for any documentation in comments about the difference
sed -n '444,450p' pyphare/pyphare/pharesee/run/utils.py && echo "---" && sed -n '493,502p' pyphare/pyphare/pharesee/run/utils.pyRepository: PHAREHUB/PHARE
Length of output: 768
🏁 Script executed:
# Let's trace through the logic: check the actual usage in hierarchy.py to understand what box is
sed -n '618,630p' pyphare/pyphare/pharesee/hierarchy/hierarchy.pyRepository: PHAREHUB/PHARE
Length of output: 708
🏁 Script executed:
# Check if PatchData box includes or excludes ghosts
rg -n "class PatchData|def box|\.box\s*=" pyphare/pyphare/pharesee/hierarchy/patchdata.py -B1 -A3 | head -60Repository: PHAREHUB/PHARE
Length of output: 346
🏁 Script executed:
# Final check: see if the +2 is consistent with including boundaries on both sides
python3 << 'EOF'
# If box represents inclusive cell indices from lower to upper
# For a 1D example: box.lower=5, box.upper=10
# Number of cells = 10 - 5 + 1 = 6 cells (indices 5,6,7,8,9,10)
# But for interpolation coordinates, we might want:
# - Points at cell boundaries: need 7 points (before cell 5, between each, after cell 10)
# That would be: upper - lower + 2
# This makes sense! The +2 gives the number of coordinate points needed
# if cells are interpreted as intervals from one point to the next
print("If cells are indexed intervals:")
print(" Box [5, 10] = 6 cells")
print(" Need 7 coordinate points for boundaries")
print(" Formula: 10 - 5 + 2 = 7 ✓")
print()
print("If cells are point locations (primal/dual on Yee grid):")
print(" Box [5, 10] = 6 cell centers")
print(" yeeCoordsFor adds +1 for primal -> 7 total")
print(" Formula should be: 10 - 5 + 1 = 6 (input), +1 inside yeeCoordsFor = 7")
EOFRepository: PHAREHUB/PHARE
Length of output: 388
🏁 Script executed:
# Check the actual yeeCoordsFor logic more carefully with the nCells parameter
sed -n '150,175p' pyphare/pyphare/core/gridlayout.py | cat -nRepository: PHAREHUB/PHARE
Length of output: 1032
🏁 Script executed:
# Look for tests that might validate build_interpolator behavior
rg -n "build_interpolator\|hierarchy.*interpol" tests/ -lRepository: PHAREHUB/PHARE
Length of output: 40
🏁 Script executed:
# Check if there are integration tests using hierarchy.interpol
rg -n "\.interpol\(" tests/ -B3 -A3 | head -40Repository: PHAREHUB/PHARE
Length of output: 40
Fix nCells calculation to match Box semantics.
The formula box.upper - box.lower + 2 adds an extra coordinate point beyond what the Box indices define. Should use box.upper - box.lower + 1 (equivalent to box.shape) since yeeCoordsFor adds 1 internally for primal centering. The current formula results in one extra coordinate extending beyond the box bounds.
🤖 Prompt for AI Agents
In @pyphare/pyphare/pharesee/run/utils.py at line 508, The nCells calculation is
off by one: change the expression that sets nCells from using "box.upper -
box.lower + 2" to use "box.upper - box.lower + 1" (or simply use "box.shape") so
it matches Box semantics and aligns with yeeCoordsFor's internal +1 for primal
centering; update the assignment where nCells is computed in utils.py (the
variable nCells and the surrounding calculation) accordingly.
There was a problem hiding this comment.
@rochSmets, understood. I've noted your feedback on this comment.
1 participant
Gaussian filter and find peaks are to be imported with
from pyphare.core.ufuncs import gF, peakIdsFor example, for a
vectorFieldlike the ion velocity withV = run.GetVi(time), it can be smoothed with a gaussian filter withsigma=6asv = gF(V, sigma=6). Hencevis a differentvectorFieldwith exactly the same structure, except that the contained dataset are smoothed.One can also look at the peaks with
pks, hgs = peakIds(v, names=['x',], height=0.015)where we need to provide the name of the dataset,valuefor example for ascalarField. In this example, the kwargheightis forwarded to thefind_peaksofscipy.ndimage. It goes the same for whatever kwargs that can be provided tofind_peaks. The returned quantities are the same, that is a tuple with the peaks location and a tuple (of same size) with their height.