Skip to content
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

nd restart tests #809

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

nd restart tests #809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6cd77a8
test bind-to-none for mpirun // tests - nd restart tests
PhilipDeegan Feb 9, 2024
f194e4d
patch order across hierarchies not guaranteed by list index
PhilipDeegan Feb 12, 2024
6bc6069
clean comment
PhilipDeegan Apr 4, 2024
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
12 changes: 8 additions & 4 deletions res/cmake/def.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ endif() # clang
set (PHARE_LINK_FLAGS )
set (PHARE_BASE_LIBS )

if(NOT DEFINED PHARE_MPIRUN_POSTFIX)
Copy link
Member Author

Choose a reason for hiding this comment

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

without this, running multiple instances of mpirun -n 2 python3 ... effectively only lets run one process, the rest are descheduled until the first is finished

we could not default it here but set it manually in TC builds, or have it as an option defaulted to ON

Copy link
Member Author

Choose a reason for hiding this comment

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

this was moved to the configurator, so to rm

set (PHARE_MPIRUN_POSTFIX --bind-to none)
endif()

if(PGO_GEN)
if(PGO_USE)
message(FATAL_ERROR "cannot generate and use pgo at the same time.")
Expand Down Expand Up @@ -181,17 +185,17 @@ if (test AND ${PHARE_EXEC_LEVEL_MIN} GREATER 0) # 0 = no tests

if(testMPI)
function(add_phare_test binary directory)
add_test(NAME ${binary} COMMAND mpirun -n ${PHARE_MPI_PROCS} ./${binary} WORKING_DIRECTORY ${directory})
add_test(NAME ${binary} COMMAND mpirun -n ${PHARE_MPI_PROCS} ${PHARE_MPIRUN_POSTFIX} ./${binary} WORKING_DIRECTORY ${directory})
add_phare_test_(${binary} ${directory})
endfunction(add_phare_test)

function(add_python3_test name file directory)
add_test(NAME py3_${name} COMMAND mpirun -n ${PHARE_MPI_PROCS} python3 -u ${file} WORKING_DIRECTORY ${directory})
add_test(NAME py3_${name} COMMAND mpirun -n ${PHARE_MPI_PROCS} ${PHARE_MPIRUN_POSTFIX} python3 -u ${file} WORKING_DIRECTORY ${directory})
set_exe_paths_(py3_${name})
endfunction(add_python3_test)

function(add_mpi_python3_test N name file directory)
add_test(NAME py3_${name}_mpi_n_${N} COMMAND mpirun -n ${N} python3 ${file} WORKING_DIRECTORY ${directory})
add_test(NAME py3_${name}_mpi_n_${N} COMMAND mpirun -n ${N} ${PHARE_MPIRUN_POSTFIX} python3 ${file} WORKING_DIRECTORY ${directory})
set_exe_paths_(py3_${name}_mpi_n_${N})
endfunction(add_mpi_python3_test)

Expand Down Expand Up @@ -254,7 +258,7 @@ if (test AND ${PHARE_EXEC_LEVEL_MIN} GREATER 0) # 0 = no tests
else()
add_test(
NAME py3_${target}_mpi_n_${N}
COMMAND mpirun -n ${N} python3 -u ${file} ${CLI_ARGS}
COMMAND mpirun -n ${N} ${PHARE_MPIRUN_POSTFIX} python3 -u ${file} ${CLI_ARGS}
WORKING_DIRECTORY ${directory})
set_exe_paths_(py3_${target}_mpi_n_${N})
endif()
Expand Down
49 changes: 31 additions & 18 deletions tests/simulator/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import unittest

from copy import deepcopy
Copy link

Choose a reason for hiding this comment

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

Consider separating imports on individual lines for clarity.

- from datetime import datetime
+ import datetime
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
from copy import deepcopy
from copy import deepcopy
import datetime



Copy link

Choose a reason for hiding this comment

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

Multiple imports on one line.

- from datetime import datetime
+ import datetime
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

from datetime import datetime
import pyphare.pharein as ph, numpy as np
from pyphare.pharein import ElectronModel


def parse_cli_args(pop_from_sys=True):
Expand Down Expand Up @@ -30,31 +33,33 @@ def basicSimulatorArgs(dim: int, interp: int, **kwargs):
from pyphare.pharein.simulation import valid_refined_particle_nbr
from pyphare.pharein.simulation import check_patch_size

args = deepcopy(kwargs)

cells = kwargs.get("cells", [20 for i in range(dim)])
if not isinstance(cells, (list, tuple)):
cells = [cells] * dim

_, smallest_patch_size = check_patch_size(dim, interp_order=interp, cells=cells)
dl = [1.0 / v for v in cells]
b0 = [[3] * dim, [8] * dim]

args = {
"interp_order": interp,
"smallest_patch_size": smallest_patch_size,
"largest_patch_size": [20] * dim,
"time_step_nbr": 1000,
"final_time": 1.0,
"time_step": 0.001,
Copy link

Choose a reason for hiding this comment

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

Consider making time_step configurable to accommodate different simulation needs.

"boundary_types": ["periodic"] * dim,
"cells": cells,
"dl": dl,
"refinement_boxes": {"L0": {"B0": b0}},
"dl": [1.0 / v for v in cells],
"refined_particle_nbr": valid_refined_particle_nbr[dim][interp][0],
"diag_options": {},
"nesting_buffer": 0,
"strict": True,
# "strict": True,
}
for k, v in kwargs.items():
if k in args:
args[k] = v
args.update(deepcopy(kwargs))

if "refinement" not in kwargs and "refinement_boxes" not in kwargs:
b0 = [[3] * dim, [8] * dim]
args["refinement_boxes"] = {"L0": {"B0": b0}}
Comment on lines +60 to +62
Copy link

Choose a reason for hiding this comment

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

The condition to set refinement_boxes if not present introduces a hardcoded refinement box. This might not be suitable for all simulations. Consider making refinement boxes configurable or clearly document this behavior.


return args

Expand Down Expand Up @@ -139,20 +144,20 @@ def makeBasicModel(extra_pops={}):
)


def populate_simulation(dim, interp, **input):
def populate_simulation(ndim=1, interp=1, model_fn=None, diags_fn=None, **simInput):
ph.global_vars.sim = None
simulation = ph.Simulation(**basicSimulatorArgs(dim, interp, **input))
simulation = ph.Simulation(**basicSimulatorArgs(ndim, interp, **simInput))
extra_pops = {}
if "populations" in input:
for pop, vals in input["populations"].items():
if "populations" in simInput:
for pop, vals in simInput["populations"].items():
extra_pops[pop] = defaultPopulationSettings()
extra_pops[pop].update(vals)

model = makeBasicModel(extra_pops)
if "diags_fn" in input:
input["diags_fn"](model)
model = model_fn() if model_fn else makeBasicModel(extra_pops)
if diags_fn:
diags_fn(model)

ElectronModel(closure="isothermal", Te=0.12)
ph.ElectronModel(closure="isothermal", Te=0.12)
Comment on lines +147 to +160
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The verification process has identified that the function populate_simulation is still being called with the old signature in two files, which contradicts the expected update across the codebase. Additionally, the new function signature is correctly used in several instances within test_restarts.py. Here are the specific locations that need attention:

  • ./tests/simulator/test_restarts.py:424: Old signature used.
  • ./tests/simulator/data_wrangler.py:25: Old signature used.

It is recommended to update these calls to match the new function signature to ensure consistency and functionality throughout the codebase.

Analysis chain

Ensure all calls to populate_simulation match the new signature and logic.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `populate_simulation` match the new signature.

# Test: Search for the old function signature to find omissions. Expect: No matches.
ast-grep --lang python --pattern $'populate_simulation($_, $_)'
# Test: Search for the new function signature to validate usage. Expect: Matches.
ast-grep --lang python --pattern $'populate_simulation($_, $_, $_, $_, $$$)'

Length of output: 2207


return simulation

Expand Down Expand Up @@ -260,3 +265,11 @@ def clean_up_diags_dirs(self):
for diag_dir in self.diag_dirs:
if os.path.exists(diag_dir):
shutil.rmtree(diag_dir)

def hierarchy_by_box(self, hier):
# comparing hierarchies by index isn't always right
box_hier = {i: {} for i in range(len(hier.levels()))}
for ilvl, patches in box_hier.items():
for patch in hier.level(ilvl).patches:
patches[str(patch.box)] = patch
return box_hier
Comment on lines +269 to +275
Copy link

@coderabbitai coderabbitai bot May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Add unit tests for the hierarchy_by_box method to ensure its correctness.

Would you like me to generate unit tests for the hierarchy_by_box method?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is used as part of a test. so it is tested.

Copy link

Choose a reason for hiding this comment

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

@PhilipDeegan, I've verified that the hierarchy_by_box method is indeed invoked in the tests/simulator/test_restarts.py file. Thus, it is being tested as part of the existing tests.

Thank you for the clarification!

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for invocations of the `hierarchy_by_box` method within the test files.
rg 'hierarchy_by_box' tests/

Length of output: 183

77 changes: 0 additions & 77 deletions tests/simulator/test_python_concurrent.py
View file
Open in desktop

This file was deleted.

Loading
Loading