Skip to content

3d C++ / python #591

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.

Sign up for GitHub

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

Closed
PhilipDeegan wants to merge 1 commit into from
Closed

3d C++ / python #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion .github/workflows/cmake_macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ jobs:
-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DENABLE_SAMRAI_TESTS=OFF -DCMAKE_C_COMPILER_LAUNCHER=ccache \
-DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DlowResourceTests=ON \
-DCMAKE_CXX_FLAGS="-DPHARE_DIAG_DOUBLES=1 "
-DCMAKE_CXX_FLAGS="-DPHARE_DIAG_DOUBLES=1 -DPHARE_SIMULATORS=2 "

- name: Build
working-directory: ${{runner.workspace}}/build
Expand Down
40 changes: 40 additions & 0 deletions pyphare/pyphare/core/box.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ def __eq__(self, other):
)

def __sub__(self, other):
if isinstance(other, (list, tuple)):
assert all([isinstance(item, Box) for item in other])
return remove_all(self, other)
assert isinstance(other, Box)
return remove(self, other)

Expand Down Expand Up @@ -179,5 +182,42 @@ def copy(arr, replace):
return list(boxes.values())


def remove_all(box, to_remove):
if len(to_remove) > 0:
remaining = box - to_remove[0]
for to_rm in to_remove[1:]:
tmp, remove = [], []
for i, rem in enumerate(remaining):
if rem * to_rm is not None:
remove.append(i)
tmp += rem - to_rm
for rm in reversed(remove):
del remaining[rm]
remaining += tmp
return remaining
return box
Comment on lines +185 to +198
Copy link

@coderabbitai coderabbitai bot May 28, 2024

Choose a reason for hiding this comment

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

The remove_all function provides a robust mechanism for removing multiple boxes. Consider adding unit tests to cover various scenarios, such as partial overlaps or complete containment.

Would you like me to help with creating unit tests for the remove_all function to ensure its robustness across different scenarios?




def amr_to_local(box, ref_box):
return Box(box.lower - ref_box.lower, box.upper - ref_box.lower)


def select(data, box):
return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])]
Comment on lines +206 to +207
Copy link

@coderabbitai coderabbitai bot Nov 19, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve function documentation and readability

  1. Add a docstring explaining the purpose and parameters
  2. Use more descriptive variable names
  3. Consider breaking down the complex list comprehension for better readability
 def select(data, box):
+    """
+    Select a subset of data using box dimensions.
+    
+    Args:
+        data: n-dimensional array-like object
+        box: Box instance defining the selection boundaries
+    
+    Returns:
+        Selected subset of data
+    """
-    return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])]
+    return data[tuple([slice(lower, upper + 1) 
+                      for lower, upper in zip(box.lower, box.upper)])]
📝 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.

Suggested change
def select(data, box):
return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])]
def select(data, box):
"""
Select a subset of data using box dimensions.
Args:
data: n-dimensional array-like object
box: Box instance defining the selection boundaries
Returns:
Selected subset of data
"""
return data[tuple([slice(lower, upper + 1)
for lower, upper in zip(box.lower, box.upper)])]
🧰 Tools
🪛 Ruff

207-207: Ambiguous variable name: l

(E741)


class DataSelector:
"""
can't assign return to function unless []
usage
DataSelector(data)[box] = val
"""
def __init__(self, data):
self.data = data
def __getitem__(self, box_or_slice):
if isinstance(box_or_slice, Box):
return select(self.data, box_or_slice)
return self.data[box_or_slice]

def __setitem__(self, box_or_slice, val):
self.__getitem__(box_or_slice)[:] = val
129 changes: 82 additions & 47 deletions pyphare/pyphare/pharesee/geometry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,19 +54,24 @@ def domain_border_ghost_boxes(domain_box, patches):
elif domain_box.ndim == 2:
upper_x, upper_y = domain_box.upper
return {
"bottom": Box(
(
0,
0,
),
(upper_x, ghost_box_width),
),
"top": Box((0, upper_y - ghost_box_width), (upper_x, upper_y)),
"left": Box((0, 0), (ghost_box_width, upper_y)),
"right": Box((upper_x - ghost_box_width, 0), (upper_x, upper_y)),
"bottom": Box((0, 0), (upper_x, ghost_box_width)),
"top": Box((0, upper_y - ghost_box_width), (upper_x, upper_y)),
}

raise ValueError("Unhandeled dimension")
else:
upper_x, upper_y, upper_z = domain_box.upper
return {
"left": Box((0, 0, 0), (ghost_box_width, upper_y, upper_z)),
"right": Box(
(upper_x - ghost_box_width, 0, 0), (upper_x, upper_y, upper_z)
),
"bottom": Box((0, 0, 0), (upper_x, ghost_box_width, upper_z)),
"top": Box((0, upper_y - ghost_box_width, 0), (upper_x, upper_y, upper_z)),
"front": Box((0, 0, 0), (upper_x, upper_y, ghost_box_width)),
"back": Box((0, 0, upper_z - ghost_box_width), (upper_x, upper_y, upper_z)),
}


def touch_domain_border(box, domain_box, border):
Expand All @@ -79,21 +84,29 @@ def touch_domain_border(box, domain_box, border):


def periodicity_shifts(domain_box):
shifts = {}

if domain_box.ndim == 1:
shape_x = domain_box.shape
return {
shifts = {
"left": shape_x,
"right": -shape_x,
}
shifts.update({"leftright": [shifts["left"], shifts["right"]]})

if domain_box.ndim == 2:
shape_x, shape_y = domain_box.shape
if domain_box.ndim == 3:
shape_x, shape_y, shape_z = domain_box.shape

if domain_box.ndim > 1:
shifts = {
"left": [(shape_x, 0)],
"right": [(-shape_x, 0)],
"bottom": [(0, shape_y)],
"top": [(0, -shape_y)],
}

shifts.update(
{
"bottomleft": [*shifts["left"], *shifts["bottom"], (shape_x, shape_y)],
Expand Down Expand Up @@ -134,7 +147,7 @@ def periodicity_shifts(domain_box):
shifts["topleft"][-1],
shifts["topright"][-1],
],
"bottomtopleftright": [ # one patch covers domain
"leftrightbottomtop": [ # one patch covers domain
*shifts["bottomleft"],
*shifts["topright"],
shifts["bottomright"][-1],
Expand All @@ -144,7 +157,35 @@ def periodicity_shifts(domain_box):
)

if domain_box.ndim == 3:
raise ValueError("Unhandeled dimension")
front = {
f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items()
Copy link

@coderabbitai coderabbitai bot May 28, 2024

Choose a reason for hiding this comment

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

Rename ambiguous variable names for clarity.

The variable l is used in list comprehensions but is not descriptive. Consider renaming it to a more meaningful name that indicates its purpose or contents.

- f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items()
+ f"{k}front": [(v[0], v[1], shape_z) for v in values] for k, values in shifts.items()

This change should be applied consistently wherever l is used in similar contexts.

Also applies to: 163-163, 166-166, 183-183

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.

Suggested change
f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items()
f"{k}front": [(v[0], v[1], shape_z) for v in values] for k, values in shifts.items()

}
back = {
f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items()
}

shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()}

shifts.update(front)
shifts.update(back)
shifts.update(
{
"back": [(0, 0, -shape_z)],
"front": [(0, 0, shape_z)],
"leftrightbottomtopfrontback": [
*shifts["bottomleftfront"],
*shifts["bottomrightback"],
*shifts["topleftfront"],
*shifts["toprightback"],
],
}
)

assert len(list(shifts.keys())) == len(
set(["".join(sorted(k)) for k in list(shifts.keys())])
)

shifts = {"".join(sorted(k)): l for k, l in shifts.items()}

return shifts

Expand Down Expand Up @@ -246,7 +287,7 @@ def borders_per(patch):
]

for patch_i, ref_patch in enumerate(border_patches):
in_sides = borders_per_patch[ref_patch]
in_sides = "".join(sorted(borders_per_patch[ref_patch]))
assert in_sides in shifts

for ref_pdname, ref_pd in ref_patch.patch_datas.items():
Expand Down Expand Up @@ -336,36 +377,41 @@ def get_periodic_list(patches, domain_box, n_ghosts):
shift_patch(first_patch, domain_box.shape)
sorted_patches.append(first_patch)

return sorted_patches

dbu = domain_box.upper

if dim == 2:
sides = {
"bottom": Box([0, 0], [domain_box.upper[0], 0]),
"top": Box(
[0, domain_box.upper[1]], [domain_box.upper[0], domain_box.upper[1]]
),
"left": Box([0, 0], [0, domain_box.upper[1]]),
"right": Box(
[domain_box.upper[0], 0], [domain_box.upper[0], domain_box.upper[1]]
),
"left": Box([0, 0], [0, dbu[1]]),
"right": Box([dbu[0], 0], [dbu[0], dbu[1]]),
"bottom": Box([0, 0], [dbu[0], 0]),
"top": Box([0, dbu[1]], [dbu[0], dbu[1]]),
}

shifts = periodicity_shifts(domain_box)
else:
sides = {
"left": Box([0, 0, 0], [0, dbu[1], dbu[2]]),
"right": Box([dbu[0], 0, 0], [dbu[0], dbu[1], dbu[2]]),
"bottom": Box([0, 0, 0], [dbu[0], 0, dbu[2]]),
"top": Box([0, dbu[1], 0], [dbu[0], dbu[1], dbu[2]]),
"front": Box([0, 0, 0], [dbu[0], dbu[1], 0]),
"back": Box([0, 0, dbu[2]], [dbu[0], dbu[1], dbu[2]]),
}

def borders_per(box):
return "".join(
[key for key, side in sides.items() if box * side is not None]
)
shifts = periodicity_shifts(domain_box)

for patch in patches:
in_sides = borders_per(boxm.grow(patch.box, n_ghosts))
def borders_per(box):
return "".join([key for key, side in sides.items() if box * side is not None])

if in_sides in shifts: # in_sides might be empty, so no borders
for shift in shifts[in_sides]:
patch_copy = copy(patch)
shift_patch(patch_copy, shift)
sorted_patches.append(patch_copy)
for patch in patches:
in_sides = "".join(sorted(borders_per(boxm.grow(patch.box, n_ghosts))))

if dim == 3:
raise ValueError("not yet implemented")
if in_sides in shifts: # in_sides might be empty, so no borders
for shift in shifts[in_sides]:
patch_copy = copy(patch)
shift_patch(patch_copy, shift)
sorted_patches.append(patch_copy)
Comment on lines +380 to +414
Copy link

@coderabbitai coderabbitai bot Aug 9, 2024

Choose a reason for hiding this comment

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

Avoid mutable default argument in get_periodic_list.

Using mutable default arguments can lead to unexpected behavior. Consider initializing within the function.

- def get_periodic_list(patches, domain_box, n_ghosts=[]):
+ def get_periodic_list(patches, domain_box, n_ghosts=None):
+     if n_ghosts is None:
+         n_ghosts = []
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.

Suggested change
return sorted_patches
dbu = domain_box.upper
if dim == 2:
sides = {
"bottom": Box([0, 0], [domain_box.upper[0], 0]),
"top": Box(
[0, domain_box.upper[1]], [domain_box.upper[0], domain_box.upper[1]]
),
"left": Box([0, 0], [0, domain_box.upper[1]]),
"right": Box(
[domain_box.upper[0], 0], [domain_box.upper[0], domain_box.upper[1]]
),
"left": Box([0, 0], [0, dbu[1]]),
"right": Box([dbu[0], 0], [dbu[0], dbu[1]]),
"bottom": Box([0, 0], [dbu[0], 0]),
"top": Box([0, dbu[1]], [dbu[0], dbu[1]]),
}
shifts = periodicity_shifts(domain_box)
else:
sides = {
"left": Box([0, 0, 0], [0, dbu[1], dbu[2]]),
"right": Box([dbu[0], 0, 0], [dbu[0], dbu[1], dbu[2]]),
"bottom": Box([0, 0, 0], [dbu[0], 0, dbu[2]]),
"top": Box([0, dbu[1], 0], [dbu[0], dbu[1], dbu[2]]),
"front": Box([0, 0, 0], [dbu[0], dbu[1], 0]),
"back": Box([0, 0, dbu[2]], [dbu[0], dbu[1], dbu[2]]),
}
def borders_per(box):
return "".join(
[key for key, side in sides.items() if box * side is not None]
)
shifts = periodicity_shifts(domain_box)
for patch in patches:
in_sides = borders_per(boxm.grow(patch.box, n_ghosts))
def borders_per(box):
return "".join([key for key, side in sides.items() if box * side is not None])
if in_sides in shifts: # in_sides might be empty, so no borders
for shift in shifts[in_sides]:
patch_copy = copy(patch)
shift_patch(patch_copy, shift)
sorted_patches.append(patch_copy)
for patch in patches:
in_sides = "".join(sorted(borders_per(boxm.grow(patch.box, n_ghosts))))
if dim == 3:
raise ValueError("not yet implemented")
if in_sides in shifts: # in_sides might be empty, so no borders
for shift in shifts[in_sides]:
patch_copy = copy(patch)
shift_patch(patch_copy, shift)
sorted_patches.append(patch_copy)
def get_periodic_list(patches, domain_box, n_ghosts=None):
if n_ghosts is None:
n_ghosts = []
return sorted_patches
dbu = domain_box.upper
if dim == 2:
sides = {
"left": Box([0, 0], [0, dbu[1]]),
"right": Box([dbu[0], 0], [dbu[0], dbu[1]]),
"bottom": Box([0, 0], [dbu[0], 0]),
"top": Box([0, dbu[1]], [dbu[0], dbu[1]]),
}
else:
sides = {
"left": Box([0, 0, 0], [0, dbu[1], dbu[2]]),
"right": Box([dbu[0], 0, 0], [dbu[0], dbu[1], dbu[2]]),
"bottom": Box([0, 0, 0], [dbu[0], 0, dbu[2]]),
"top": Box([0, dbu[1], 0], [dbu[0], dbu[1], dbu[2]]),
"front": Box([0, 0, 0], [dbu[0], dbu[1], 0]),
"back": Box([0, 0, dbu[2]], [dbu[0], dbu[1], dbu[2]]),
}
shifts = periodicity_shifts(domain_box)
def borders_per(box):
return "".join([key for key, side in sides.items() if box * side is not None])
for patch in patches:
in_sides = "".join(sorted(borders_per(boxm.grow(patch.box, n_ghosts))))
if in_sides in shifts: # in_sides might be empty, so no borders
for shift in shifts[in_sides]:
patch_copy = copy(patch)
shift_patch(patch_copy, shift)
sorted_patches.append(patch_copy)


return sorted_patches

Expand Down Expand Up @@ -470,18 +516,7 @@ def level_ghost_boxes(hierarchy, quantities, levelNbrs=[], time=None):
check_patches = patches

for gabox in ghostAreaBoxes:
remaining = gabox - check_patches[0].box

for patch in check_patches[1:]:
tmp = []
remove = []
for i, rem in enumerate(remaining):
if rem * patch.box is not None:
remove.append(i)
tmp += rem - patch.box
for rm in reversed(remove):
del remaining[rm]
remaining += tmp
remaining = gabox - [p.box for p in check_patches]

if ilvl not in lvl_gaboxes:
lvl_gaboxes[ilvl] = {}
Expand Down
33 changes: 33 additions & 0 deletions pyphare/pyphare/pharesee/hierarchy/hierarchy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,11 +556,44 @@ def plot2d(self, **kwargs):

return fig, ax

def plot3d(self, **kwargs):
"""!HAX!"""
Copy link
Member

@nicolasaunai nicolasaunai Feb 7, 2025

Choose a reason for hiding this comment

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

yeah.... not sure exactly this function should stay there. If I understand, it's basically hacking the 3D hierarchy into a 2D one that is the slice at the middle Z index.
I think this should not be called plot3d. Rather, we should do allow for making a slice in a n-dimensional hierarchy at a certain index and then use the normal plot2d.
But this is not as simple. Already, making a 1D slice in a 2D hierarchy is complicated since fine indexes are not aligned with coarse one. So making a 2D slice in 3D has the same issue.

Copy link
Member Author

@PhilipDeegan PhilipDeegan Feb 10, 2025

Choose a reason for hiding this comment

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

I was using this just to check the 3d run wasn't total nonsense

time = kwargs.get("time", self._default_time())
Comment on lines +559 to +561
Copy link

@coderabbitai coderabbitai bot Feb 10, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve documentation and remove "!HAX!" comment.

The method lacks proper documentation. Replace the "!HAX!" comment with a proper docstring explaining the method's purpose, parameters, and behavior.

-    def plot3d(self, **kwargs):
-        """!HAX!"""
+    def plot3d(self, **kwargs):
+        """Plot 3D data by taking a 2D slice at the middle Z index.
+        
+        Args:
+            **kwargs: Keyword arguments passed to plot2d.
+        
+        Returns:
+            The result of plot2d.
+        
+        Note:
+            This is a temporary solution that modifies the input data in-place.
+            TODO: Implement proper 3D visualization or slicing without data modification.
+        """
📝 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.

Suggested change
def plot3d(self, **kwargs):
"""!HAX!"""
time = kwargs.get("time", self._default_time())
def plot3d(self, **kwargs):
"""Plot 3D data by taking a 2D slice at the middle Z index.
Args:
**kwargs: Keyword arguments passed to plot2d.
Returns:
The result of plot2d.
Note:
This is a temporary solution that modifies the input data in-place.
TODO: Implement proper 3D visualization or slicing without data modification.
"""
time = kwargs.get("time", self._default_time())

usr_lvls = kwargs.get("levels", self.levelNbrs(time))
default_qty = None
if len(self.quantities()) == 1:
default_qty = self.quantities()[0]
qty = kwargs.get("qty", default_qty)
for lvl_nbr, lvl in self.levels(time).items():
if lvl_nbr not in usr_lvls:
continue
for patch in self.level(lvl_nbr, time).patches:
pdat = patch.patch_datas[qty]
primals = pdat.primal_directions()
if primals[0]:
pdat._x = pdat.x[:-1]
if primals[1]:
pdat._y = pdat.y[:-1]
pdat.dataset = pdat.dataset[:, :, int(pdat.ghost_box.shape[2] / 2)]
patch.box.lower = patch.box.lower[:-1]
patch.box.upper = patch.box.upper[:-1]
patch.box.ndim = 2

pdat.ghost_box.lower = pdat.ghost_box.lower[:-1]
pdat.ghost_box.upper = pdat.ghost_box.upper[:-1]
pdat.ghost_box.ndim = 2
pdat.size = np.copy(pdat.ghost_box.shape)
pdat.layout.dl = pdat.layout.dl[:-1]

Comment on lines +571 to +587
Copy link

@coderabbitai coderabbitai bot Feb 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid in-place data modification and hardcoded slice selection.

The method modifies the input data in-place and uses a hardcoded middle Z index for slicing. This can lead to side effects and limits flexibility.

Consider creating a new method that:

  1. Creates a copy of the data
  2. Allows configurable slice selection
  3. Preserves the original data
-                pdat = patch.patch_datas[qty]
-                primals = pdat.primal_directions()
-                if primals[0]:
-                    pdat._x = pdat.x[:-1]
-                if primals[1]:
-                    pdat._y = pdat.y[:-1]
-                pdat.dataset = pdat.dataset[:, :, int(pdat.ghost_box.shape[2] / 2)]
-                patch.box.lower = patch.box.lower[:-1]
-                patch.box.upper = patch.box.upper[:-1]
-                patch.box.ndim = 2
-
-                pdat.ghost_box.lower = pdat.ghost_box.lower[:-1]
-                pdat.ghost_box.upper = pdat.ghost_box.upper[:-1]
-                pdat.ghost_box.ndim = 2
-                pdat.size = np.copy(pdat.ghost_box.shape)
-                pdat.layout.dl = pdat.layout.dl[:-1]
+                pdat = patch.patch_datas[qty]
+                slice_data = self._create_2d_slice(pdat, kwargs.get('z_index', None))
+                patch_2d = self._create_2d_patch(patch, slice_data)

Would you like me to generate the implementation for the helper methods _create_2d_slice and _create_2d_patch?

Committable suggestion skipped: line range outside the PR's diff.

return self.plot2d(**kwargs) # ¯\_(ツ)_/¯

def plot(self, **kwargs):
if self.ndim == 1:
return self.plot1d(**kwargs)
elif self.ndim == 2:
return self.plot2d(**kwargs)
elif self.ndim == 3:
return self.plot3d(**kwargs)

def dist_plot(self, **kwargs):
"""
Expand Down
2 changes: 1 addition & 1 deletion pyphare/pyphare/pharesee/run/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ def _get_rank(patchdatas, patch_id, **kwargs):
layout = reference_pd.layout
centering = "dual"
nbrGhosts = layout.nbrGhosts(layout.interp_order, centering)
shape = grow(reference_pd.box, [nbrGhosts] * 2).shape
shape = grow(reference_pd.box, [nbrGhosts] * ndim).shape

if ndim == 1:
pass
Expand Down
40 changes: 40 additions & 0 deletions res/amr/splitting.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,43 @@ dimension_2:
delta: 1 2
weight: .140625 .09375 .0625 .0234375 .015625 .00390625


dimension_3:
interp_1:
N_particles_6:
delta: .966431
weight: .166666

N_particles_12:
delta: .74823
weight: .083333

N_particles_27:
delta: 1
weight: .125 .0625

interp_2:
N_particles_6:
delta: 1.149658
weight: .166666

N_particles_12:
delta: .888184
weight: .083333

N_particles_27:
delta: 1.111333
weight: .099995 .055301

interp_3:
N_particles_6:
delta: 1.312622
weight: .166666

N_particles_12:
delta: 1.012756
weight: .083333

N_particles_27:
delta: 1.276815
weight: .104047 .05564
10 changes: 10 additions & 0 deletions res/cmake/options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,19 @@ option(withPhlop "Use phlop" OFF)
# -DlowResourceTests=ON
option(lowResourceTests "Disable heavy tests for CI (2d/3d/etc" OFF)

# -DhighResourceTests=ON
option(highResourceTests "Enable heavy tests for CI (3d/etc" OFF)

Comment on lines +73 to +75
Copy link

@coderabbitai coderabbitai bot Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ensure mutual exclusivity between lowResourceTests and highResourceTests.

The new highResourceTests option is useful for enabling heavy tests in CI. However, ensure that lowResourceTests and highResourceTests cannot be enabled simultaneously to avoid conflicting configurations.

+ if(lowResourceTests AND highResourceTests)
+   message(FATAL_ERROR "Both lowResourceTests and highResourceTests cannot be enabled at the same time.")
+ endif()
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.

Suggested change
# -DhighResourceTests=ON
option(highResourceTests "Enable heavy tests for CI (3d/etc" OFF)
# -DhighResourceTests=ON
option(highResourceTests "Enable heavy tests for CI (3d/etc" OFF)
if(lowResourceTests AND highResourceTests)
message(FATAL_ERROR "Both lowResourceTests and highResourceTests cannot be enabled at the same time.")
endif()

Copy link
Member

@nicolasaunai nicolasaunai Feb 7, 2025

Choose a reason for hiding this comment

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

I agree with the rabbit, no?

Copy link

@coderabbitai coderabbitai bot Feb 7, 2025

Choose a reason for hiding this comment

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

@nicolasaunai Thanks for confirming! I'll add this to my knowledge base to ensure future reviews maintain this mutual exclusivity requirement between the test options.

🐰 thoughtfully nods in agreement

✏️ Learnings added 
Learnt from: nicolasaunai
PR: PHAREHUB/PHARE#591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.365Z
Learning: In CMake configuration for PHARE, `lowResourceTests` and `highResourceTests` options must be mutually exclusive and cannot be enabled simultaneously.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Member Author

@PhilipDeegan PhilipDeegan Feb 10, 2025

Choose a reason for hiding this comment

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

feasible there's a better way not involving either flag

Copy link

@coderabbitai coderabbitai bot Feb 10, 2025

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

if(lowResourceTests AND highResourceTests)
message(FATAL_ERROR "Both lowResourceTests and highResourceTests cannot be enabled at the same time.")
endif()


# -DtestDuringBuild=ON enabled if devMode=ON, disabled if asan=ON (needs LD_PRELOAD)
option(testDuringBuild "Runs C++ unit tests after they are built" OFF)



Copy link

@coderabbitai coderabbitai bot Nov 19, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing print statement for highResourceTests option.

The new option's status should be displayed when print_phare_options is called.

Add this line in the print_phare_options function, next to the lowResourceTests print statement:

     message("Exclude heavier tests                       : " ${lowResourceTests})
+    message("Include resource-intensive tests            : " ${highResourceTests})
     message("PHARE_MPI_PROCS                             : " ${PHARE_MPI_PROCS})

Committable suggestion skipped: line range outside the PR's diff.

# -DPGO_GEN=OFF profile guided optimization generate
option(PGO_GEN "profile guided optimization generate" OFF)
# -DPGO_USE=OFF
Expand All @@ -84,6 +93,7 @@ option(PGO_USE "profile guided optimization use" OFF)
option(PHARE_COMPILER_WORKAROUNDS "Attempt silence compiler false positives" ON) # on by default



# Controlling the activation of tests
if (NOT DEFINED PHARE_EXEC_LEVEL_MIN)
set(PHARE_EXEC_LEVEL_MIN 1)
Expand Down
Loading
Loading