modify the Getter of run.py to manage merged=True#1148
modify the Getter of run.py to manage merged=True#1148rochSmets wants to merge 2 commits intoPHAREHUB:masterfrom
Conversation
📝 WalkthroughWalkthroughThis pull request extends centering metadata support across the physics simulation framework. It expands the Yee centering lookup table with new quantity entries, updates how operators reference centering data, adds parametric control for primal computation in data retrieval methods, and includes minor visualization and documentation updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant GetX as GetX Method<br/>(GetB/GetE/GetN/etc)
participant Hierarchy as Hierarchy Data
participant Primal as Primal Computation
participant Field as Field Object
User->>GetX: call with all_primal=True/False
alt all_primal is False
GetX->>Hierarchy: _get(use existing hierarchy)
Hierarchy->>GetX: return raw hierarchical data
GetX->>User: return data
else all_primal is True
GetX->>Hierarchy: retrieve hierarchy data
Hierarchy->>GetX: return hierarchy
GetX->>GetX: optionally rename keys<br/>(e.g., rho, Fx/Fy/Fz)
GetX->>Primal: compute primal representation
Primal->>Field: create ScalarField/VectorField
Field->>User: return primal Field
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🧪 Generate unit tests (beta)
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}
| if not all_primal: | ||
| return self._get(hier, time, merged, interp) | ||
|
|
||
| h = compute_hier_from(_compute_to_primal, hier, x="Vx", y="Vy", z="Vz") |
Check notice
Code scanning / CodeQL
Unused local variable Note
| if not all_primal: | ||
| return self._get(hier_, time, merged, interp) | ||
|
|
||
| h = compute_hier_from(_compute_to_primal, hier_, x="Fx", y="Fy", z="Fz") |
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyphare/pyphare/pharesee/run/run.py (1)
194-203:⚠️ Potential issue | 🟠 MajorUse
value="rho"instead ofscalar="rho"for consistency.Line 202 uses
scalar="rho"but the correct kwarg name isvalue="rho", as demonstrated byGetMassDensity,GetNi, andGetNwhich all follow the same pattern for scalar field computations.Fix
if not all_primal: return Te * self._get(hier, time, merged, interp) - h = compute_hier_from(_compute_to_primal, hier, scalar="rho") + h = compute_hier_from(_compute_to_primal, hier, value="rho") return ScalarField(h) * Te
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/pharesee/run/run.py`:
- Around line 149-158: The GetVi method computes a primal hierarchy h via
compute_hier_from(_compute_to_primal, hier, x="Vx", y="Vy", z="Vz") but then
ignores it and calls self._get on the original hier; change the final return to
use h so the primal hierarchy is actually retrieved (i.e., call self._get with h
instead of hier) so VectorField wraps the data derived from compute_hier_from;
references: GetVi, compute_hier_from, _compute_to_primal, h, hier, self._get,
VectorField.
- Around line 160-171: GetFlux computes a primal hierarchy h via
compute_hier_from(_compute_to_primal, ...) but never uses it and then returns
VectorField(self._get(hier, ...)) which is both the wrong hierarchy variable and
returns un-renamed keys; change the return to use the computed primal hierarchy
(and the renamed hierarchy where appropriate): call self._get on h (or on hier_
if you intend the renamed-but-not-primal data) and wrap that result in
VectorField so the function returns the computed primal/renamed flux data;
update the return statement in GetFlux accordingly (references: GetFlux,
_get_hierarchy, rename, compute_hier_from, _compute_to_primal, VectorField,
self._get).
🧹 Nitpick comments (2)
pyphare/pyphare/core/operators.py (1)
22-29: Consider removing the commented-out code.The TODO comment is helpful context, but the commented-out assignment could be removed entirely since it's now dead code. The comment alone would suffice to document the assumption.
🧹 Suggested cleanup
def _compute_sqrt(patch_datas, **kwargs): - # ref_name = next(iter(patch_datas.keys())) TODO this is always "value" + # Note: patch_datas always contains "value" key when used via sqrt() dset = np.sqrt(patch_datas["value"][:])pyphare/pyphare/pharesee/run/utils.py (1)
312-312: Remove commented-out debug code.Debug print statements should be removed rather than commented out in production code.
🧹 Suggested cleanup
pd_attrs = [] for name, pd_name in kwargs.items(): - # print(name, pd_name) pd = patchdatas[pd_name]
| def GetVi(self, time, merged=False, interp="nearest", all_primal=True, **kwargs): | ||
| if merged: | ||
| all_primal = False | ||
| hier = self._get_hierarchy(time, "ions_bulkVelocity.h5", **kwargs) | ||
|
|
||
| if not all_primal: | ||
| return self._get(hier, time, merged, interp) | ||
|
|
||
| h = compute_hier_from(_compute_to_primal, hier, x="Vx", y="Vy", z="Vz") | ||
| return VectorField(self._get(hier, time, merged, interp)) |
There was a problem hiding this comment.
Critical bug: Computed h is never used.
The primal hierarchy h is computed on line 157 but the return statement on line 158 ignores it and returns a VectorField wrapping the original hier instead. This means all_primal=True does not actually return primal data.
🐛 Proposed fix
def GetVi(self, time, merged=False, interp="nearest", all_primal=True, **kwargs):
if merged:
all_primal = False
hier = self._get_hierarchy(time, "ions_bulkVelocity.h5", **kwargs)
if not all_primal:
return self._get(hier, time, merged, interp)
h = compute_hier_from(_compute_to_primal, hier, x="Vx", y="Vy", z="Vz")
- return VectorField(self._get(hier, time, merged, interp))
+ return VectorField(h)🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 157-157: Unused local variable
Variable h is not used.
🪛 Ruff (0.14.14)
[error] 157-157: Local variable h is assigned to but never used
Remove assignment to unused variable h
(F841)
🤖 Prompt for AI Agents
In `@pyphare/pyphare/pharesee/run/run.py` around lines 149 - 158, The GetVi method
computes a primal hierarchy h via compute_hier_from(_compute_to_primal, hier,
x="Vx", y="Vy", z="Vz") but then ignores it and calls self._get on the original
hier; change the final return to use h so the primal hierarchy is actually
retrieved (i.e., call self._get with h instead of hier) so VectorField wraps the
data derived from compute_hier_from; references: GetVi, compute_hier_from,
_compute_to_primal, h, hier, self._get, VectorField.
| def GetFlux(self, time, pop_name, merged=False, interp="nearest", all_primal=True, **kwargs): | ||
| if merged: | ||
| all_primal = False | ||
| hier = self._get_hierarchy(time, f"ions_pop_{pop_name}_flux.h5", **kwargs) | ||
|
|
||
| hier_ = rename(hier, ['Fx', 'Fy', 'Fz']) | ||
| # print(hier.quantities()) | ||
| if not all_primal: | ||
| return self._get(hier_, time, merged, interp) | ||
|
|
||
| h = compute_hier_from(_compute_to_primal, hier_, x="Fx", y="Fy", z="Fz") | ||
| return VectorField(self._get(hier, time, merged, interp)) |
There was a problem hiding this comment.
Critical bug: Computed h is never used and wrong hierarchy returned.
Similar to GetVi, line 170 computes primal hierarchy h but line 171 ignores it. Additionally, line 171 uses hier instead of hier_ (the renamed hierarchy), which would return data with the original keys instead of Fx, Fy, Fz.
🐛 Proposed fix
def GetFlux(self, time, pop_name, merged=False, interp="nearest", all_primal=True, **kwargs):
if merged:
all_primal = False
hier = self._get_hierarchy(time, f"ions_pop_{pop_name}_flux.h5", **kwargs)
hier_ = rename(hier, ['Fx', 'Fy', 'Fz'])
- # print(hier.quantities())
if not all_primal:
return self._get(hier_, time, merged, interp)
h = compute_hier_from(_compute_to_primal, hier_, x="Fx", y="Fy", z="Fz")
- return VectorField(self._get(hier, time, merged, interp))
+ return VectorField(h)🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 170-170: Unused local variable
Variable h is not used.
🪛 Ruff (0.14.14)
[error] 170-170: Local variable h is assigned to but never used
Remove assignment to unused variable h
(F841)
🤖 Prompt for AI Agents
In `@pyphare/pyphare/pharesee/run/run.py` around lines 160 - 171, GetFlux computes
a primal hierarchy h via compute_hier_from(_compute_to_primal, ...) but never
uses it and then returns VectorField(self._get(hier, ...)) which is both the
wrong hierarchy variable and returns un-renamed keys; change the return to use
the computed primal hierarchy (and the renamed hierarchy where appropriate):
call self._get on h (or on hier_ if you intend the renamed-but-not-primal data)
and wrap that result in VectorField so the function returns the computed
primal/renamed flux data; update the return statement in GetFlux accordingly
(references: GetFlux, _get_hierarchy, rename, compute_hier_from,
_compute_to_primal, VectorField, self._get).
|
I think long term we are going to change how this "merging" is done |
|
|
||
| def _compute_sqrt(patch_datas, **kwargs): | ||
| ref_name = next(iter(patch_datas.keys())) | ||
| # ref_name = next(iter(patch_datas.keys())) TODO this is always "value" |
2 participants
The getter that we have in
run.py(GetB,GetN, ...) can handle the kwargmerged:True, then we get an interpolatorFalse, we get a hierarchy. But ifall_primalisTrue, this hierarchy is the now aScalarFieldor aVectorField