Add directional, MPI, restart, and kernel regression tests#1213
Draft
sbryngelson wants to merge 7 commits intoMFlowCode:masterfrom
Draft
Add directional, MPI, restart, and kernel regression tests#1213sbryngelson wants to merge 7 commits intoMFlowCode:masterfrom
sbryngelson wants to merge 7 commits intoMFlowCode:masterfrom
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional per-case restart verification via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Harness as "Test Harness"
participant Case as "TestCase"
participant Sim as "Simulation (PRE_PROCESS → SIMULATION)"
participant FS as "Filesystem / Packer"
participant Compare as "Comparator"
rect rgba(200,200,255,0.5)
Harness->>Case: construct with restart_check=True
end
rect rgba(200,255,200,0.5)
Harness->>Sim: run case to midpoint
Sim-->>FS: write restart checkpoint files
FS-->>Harness: checkpoint available
end
rect rgba(255,200,200,0.5)
Harness->>FS: prune non-essential outputs
Harness->>Sim: restart from checkpoint and continue
Sim-->>FS: write final outputs
end
rect rgba(200,255,255,0.5)
FS->>Compare: pack restarted outputs
FS->>Compare: pack original outputs
Compare-->>Harness: run comparison (tolerance, NaN checks)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 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 |
codeant-ai
bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Contributor
Nitpicks 🔍
|
Contributor
|
CodeAnt AI finished reviewing your PR. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 4/5
- The only noted issue is a missing post-run timeout check in
toolchain/mfc/test/test.py, which could allow a hung restart test to pass the 1-hour guard and mask test failures. - Overall risk seems limited to test reliability rather than production behavior, so this looks safe to merge with a small caveat.
- Pay close attention to
toolchain/mfc/test/test.py- add a timeout check afterrun_restartto catch long/hung runs.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="toolchain/mfc/test/test.py">
<violation number="1" location="toolchain/mfc/test/test.py:395">
P2: Timeouts triggered during the restart roundtrip won’t be detected because there’s no timeout check after `run_restart` completes. This can let long/hung restart runs pass the 1‑hour guard. Add the same post-run timeout check used elsewhere.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
sbryngelson
force-pushed
the
better-tests
branch
from
8d19a79 to
f3f1f47
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds new regression coverage to the MFC test toolchain to catch direction-dependent, MPI-rank-dependent, restart I/O, and specific kernel indexing issues.
Changes:
- Added new generated test cases for direction symmetry, MPI ppn=2 consistency, restart roundtrip, and a polydisperse bubbles kernel golden.
- Extended
TestCase/TestCaseBuilderto support arestart_checkflag and implementedrun_restart()to perform a two-phase run. - Updated the test runner to perform restart roundtrip verification after a successful golden comparison.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| toolchain/mfc/test/test.py | Runs restart roundtrip verification and compares restarted outputs to the straight-run pack. |
| toolchain/mfc/test/cases.py | Adds new generated test cases for direction symmetry, MPI consistency, restart roundtrip, and kernel golden coverage. |
| toolchain/mfc/test/case.py | Introduces restart_check plumbing and implements run_restart() for two-phase restart verification. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
toolchain/mfc/test/cases.py (1)
1151-1237: LGTM! MPI consistency tests cover three distinct physics modules (bubbles, viscous, hypoelasticity) at ppn=2. The hypoelasticity variant correctly includes all 6tau_ecomponents for the 3D domain. Stack push/pop is balanced for all three sub-tests.One minor observation: the
base_3ddict construction (lines 1157–1180) is nearly identical to the one inrestart_roundtrip_tests(lines 1263–1285). If you find yourself adding more 3D test suites, consider extracting a helper, but for now this is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/cases.py` around lines 1151 - 1237, The base_3d dictionary in mpi_consistency_tests is nearly identical to the base_3d in restart_roundtrip_tests; extract that common dictionary construction into a single helper (e.g., new function build_base_3d()) and replace the inline base_3d definitions in mpi_consistency_tests and restart_roundtrip_tests with calls to build_base_3d(), keeping all subsequent per-test updates (patch entries and stack.push calls) unchanged; locate the code by referencing the function names mpi_consistency_tests, restart_roundtrip_tests and the symbol base_3d to make the replacement.toolchain/mfc/test/test.py (1)
389-389: Prefer direct attribute access overgetattrfor a declared field.
restart_checkis a declared field onTestCase(with defaultFalse), socase.restart_checkalways exists. Usinggetattr(case, 'restart_check', False)suggests the attribute might be missing, which is misleading.♻️ Suggested simplification
- if getattr(case, 'restart_check', False) and not ARG("add_new_variables"): + if case.restart_check and not ARG("add_new_variables"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/test.py` at line 389, The condition uses getattr(case, 'restart_check', False) even though restart_check is a declared TestCase field with a default; replace the getattr call with direct attribute access (case.restart_check) in the conditional that also checks ARG("add_new_variables") so it becomes: if case.restart_check and not ARG("add_new_variables"): — update the expression where the variable case and field restart_check are used to improve clarity and avoid the misleading fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@toolchain/mfc/test/test.py`:
- Around line 387-413: After completing the restart roundtrip (the block that
calls case.run_restart and repacks), overwrite the on-disk case.py with the
restored in-memory params so subsequent calls to case.run() use the correct
Phase 1 params; specifically, after the restart block and before the subsequent
ARG("test_all") / case.run() logic, write the current in-memory case parameters
back to disk (e.g., regenerate/overwrite case.py under case.get_dirpath()) so
the stale Phase 2 params (old_ic, old_grid, t_step_start) are replaced.
- Line 395: run_restart is missing type annotations for its gpus parameter;
update its signature to match run by annotating gpus as typing.Set[int] (same as
run at line 120) so calls from _handle_case passing devices (typed as
typing.Set[int]) remain type-safe—locate the run_restart definition and add the
gpus: Set[int] annotation and import typing.Set if not already imported.
---
Nitpick comments:
In `@toolchain/mfc/test/cases.py`:
- Around line 1151-1237: The base_3d dictionary in mpi_consistency_tests is
nearly identical to the base_3d in restart_roundtrip_tests; extract that common
dictionary construction into a single helper (e.g., new function
build_base_3d()) and replace the inline base_3d definitions in
mpi_consistency_tests and restart_roundtrip_tests with calls to build_base_3d(),
keeping all subsequent per-test updates (patch entries and stack.push calls)
unchanged; locate the code by referencing the function names
mpi_consistency_tests, restart_roundtrip_tests and the symbol base_3d to make
the replacement.
In `@toolchain/mfc/test/test.py`:
- Line 389: The condition uses getattr(case, 'restart_check', False) even though
restart_check is a declared TestCase field with a default; replace the getattr
call with direct attribute access (case.restart_check) in the conditional that
also checks ARG("add_new_variables") so it becomes: if case.restart_check and
not ARG("add_new_variables"): — update the expression where the variable case
and field restart_check are used to improve clarity and avoid the misleading
fallback.
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:L
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
toolchain/mfc/test/test.py (1)
389-389: Prefer direct attribute access overgetattrwith a default.
restart_checkis a declaredbool = Falsefield onTestCase, sogetattr(case, 'restart_check', False)is unnecessarily defensive.♻️ Proposed simplification
- if getattr(case, 'restart_check', False) and not ARG("add_new_variables"): + if case.restart_check and not ARG("add_new_variables"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/test.py` at line 389, The getattr call is unnecessarily defensive because TestCase declares restart_check: bool = False; replace getattr(case, 'restart_check', False) with direct attribute access case.restart_check in the conditional (the if using case.restart_check and not ARG("add_new_variables")). Update any similar occurrences tied to the TestCase instance to use the restart_check attribute directly to keep the intent clear and avoid redundant defaults.toolchain/mfc/test/case.py (1)
113-114: UseT | Noneunion syntax for optional parameters (Ruff RUF013).PEP 484 prohibits implicit
Optionalvia= Nonedefaults for non-Optional-typed params. With Python 3.10+ required, the union syntax should be used.♻️ Proposed fix
- def __init__(self, trace: str, mods: dict, ppn: int = None, override_tol: float = None, restart_check: bool = False) -> None: + def __init__(self, trace: str, mods: dict, ppn: int | None = None, override_tol: float | None = None, restart_check: bool = False) -> None:Same applies to
define_case_dat line 369:-def define_case_d(stack: CaseGeneratorStack, newTrace: str, newMods: dict, ppn: int = None, functor: Callable = None, override_tol: float = None, restart_check: bool = False) -> TestCaseBuilder: +def define_case_d(stack: CaseGeneratorStack, newTrace: str, newMods: dict, ppn: int | None = None, functor: Callable | None = None, override_tol: float | None = None, restart_check: bool = False) -> TestCaseBuilder:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` around lines 113 - 114, Change the function signatures to use explicit union types (PEP 604) for parameters that default to None: update __init__ to annotate ppn as int | None and override_tol as float | None (leave restart_check as bool), and apply the same pattern in define_case_d (convert any parameters typed like X with default None to X | None). Locate the signatures by the unique symbols __init__ and define_case_d and replace the non-Optional typed params that use = None with the T | None union syntax to satisfy Ruff RUF013.toolchain/mfc/test/cases.py (1)
1157-1180: Duplicated 3D-base setup acrossmpi_consistency_tests,restart_roundtrip_tests, andkernel_golden_tests.The domain/BC/patch-geometry block is copy-pasted verbatim three times. The only variation is the grid size (
29×29×49for MPI tests vs24×24×24for the others).♻️ Suggested helper extraction
Add a helper inside
list_cases:def make_3d_shock_base(m=24, n=24, p=24): base = { 'm': m, 'n': n, 'p': p, 'x_domain%beg': 0.E+00, 'x_domain%end': 1.E+00, 'y_domain%beg': 0.E+00, 'y_domain%end': 1.E+00, 'z_domain%beg': 0.E+00, 'z_domain%end': 1.E+00, 'bc_x%beg': -3, 'bc_x%end': -3, 'bc_y%beg': -3, 'bc_y%end': -3, 'bc_z%beg': -3, 'bc_z%end': -3, } for pid in range(1, 4): base[f'patch_icpp({pid})%geometry'] = 9 base[f'patch_icpp({pid})%vel(1)'] = 0.0 base[f'patch_icpp({pid})%vel(2)'] = 0.0 base[f'patch_icpp({pid})%vel(3)'] = 0.0 base[f'patch_icpp({pid})%x_centroid'] = 0.5 base[f'patch_icpp({pid})%length_x'] = 1 base[f'patch_icpp({pid})%y_centroid'] = 0.5 base[f'patch_icpp({pid})%length_y'] = 1 base.update({ 'patch_icpp(1)%z_centroid': 0.05, 'patch_icpp(1)%length_z': 0.1, 'patch_icpp(2)%z_centroid': 0.45, 'patch_icpp(2)%length_z': 0.7, 'patch_icpp(3)%z_centroid': 0.9, 'patch_icpp(3)%length_z': 0.2, }) return baseThen replace each duplicated block with
base_3d = make_3d_shock_base()(ormake_3d_shock_base(m=29, n=29, p=49)for the MPI case).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/cases.py` around lines 1157 - 1180, Extract the repeated 3D domain/BC/patch-geometry block into a helper function (e.g., make_3d_shock_base) inside list_cases that returns a dict and accepts parameters m,n,p (default 24) and then replace the three copy-pasted constructions with calls to that helper (use make_3d_shock_base(m=29,n=29,p=49) for the MPI case); ensure the helper populates the same keys (base_3d, patch_icpp(<pid>)%geometry, vel(1..3), x_centroid/length_x, y_centroid/length_y and the three z_centroid/length_z entries) so existing code that references base_3d and patch_icpp(...) continues to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@toolchain/mfc/test/case.py`:
- Around line 148-172: mid_step currently uses self.params['t_step_stop'] // 2
which can produce mid < orig['t_step_start'] and create invalid Phase 1 params;
change mid_step to bisect the actual interval, e.g. compute mid_step =
(orig['t_step_start'] + orig['t_step_stop']) // 2 (using the original params
dict), then use orig for Phase 1 so t_step_start stays orig['t_step_start'] and
t_step_stop becomes mid_step, and for Phase 2 set t_step_start = mid_step with
t_step_save = orig['t_step_stop'] - mid_step; update references to mid_step,
params, and the Phase 1/Phase 2 setups around create_directory() and run()
accordingly.
---
Duplicate comments:
In `@toolchain/mfc/test/case.py`:
- Line 146: The method run_restart lacks type annotations; update its signature
to include explicit parameter and return types (for example: change def
run_restart(self, targets, gpus) to def run_restart(self, targets:
Sequence[str], gpus: Sequence[int]) -> None) and add the necessary typing import
(from typing import Sequence) at the top of the module, or adjust the concrete
types to match actual usage (List[str], int, Optional[...] or bool) if the
function semantics require different types; ensure the annotation matches usages
of run_restart elsewhere in the codebase.
In `@toolchain/mfc/test/test.py`:
- Line 395: The call to case.run_restart has previously flagged incorrect or
missing type annotations; update the run_restart function signature (the
run_restart method on the Case/CaseRunner class) to have precise parameter and
return types (e.g., devices: Sequence[Device] or List[Device], stages:
Sequence[Stage] or List[str] as appropriate) and a concrete return type (not
Any) so the assignment restart_result has a correct inferred type; then adjust
any usages (including this call site) to pass matching typed values and import
the Device/Stage types or use typing.Protocol/TypeAlias if needed.
---
Nitpick comments:
In `@toolchain/mfc/test/case.py`:
- Around line 113-114: Change the function signatures to use explicit union
types (PEP 604) for parameters that default to None: update __init__ to annotate
ppn as int | None and override_tol as float | None (leave restart_check as
bool), and apply the same pattern in define_case_d (convert any parameters typed
like X with default None to X | None). Locate the signatures by the unique
symbols __init__ and define_case_d and replace the non-Optional typed params
that use = None with the T | None union syntax to satisfy Ruff RUF013.
In `@toolchain/mfc/test/cases.py`:
- Around line 1157-1180: Extract the repeated 3D domain/BC/patch-geometry block
into a helper function (e.g., make_3d_shock_base) inside list_cases that returns
a dict and accepts parameters m,n,p (default 24) and then replace the three
copy-pasted constructions with calls to that helper (use
make_3d_shock_base(m=29,n=29,p=49) for the MPI case); ensure the helper
populates the same keys (base_3d, patch_icpp(<pid>)%geometry, vel(1..3),
x_centroid/length_x, y_centroid/length_y and the three z_centroid/length_z
entries) so existing code that references base_3d and patch_icpp(...) continues
to work.
In `@toolchain/mfc/test/test.py`:
- Line 389: The getattr call is unnecessarily defensive because TestCase
declares restart_check: bool = False; replace getattr(case, 'restart_check',
False) with direct attribute access case.restart_check in the conditional (the
if using case.restart_check and not ARG("add_new_variables")). Update any
similar occurrences tied to the TestCase instance to use the restart_check
attribute directly to keep the intent clear and avoid redundant defaults.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
toolchain/mfc/test/case.py (3)
113-114: Use explicitT | Noneunion syntax to satisfy PEP 484 (Ruff RUF013)
ppn: int = Noneandoverride_tol: float = Noneare implicitOptional— prohibited by PEP 484. With Python 3.10+ already required by this toolchain, prefer the union syntax.♻️ Proposed fix
- def __init__(self, trace: str, mods: dict, ppn: int = None, override_tol: float = None, restart_check: bool = False) -> None: + def __init__(self, trace: str, mods: dict, ppn: int | None = None, override_tol: float | None = None, restart_check: bool = False) -> None:As per coding guidelines: "Python toolchain code. Follow PEP 8. Requires Python 3.10+."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` around lines 113 - 114, The __init__ signature in class Case (function __init__) uses implicit Optional defaults (ppn: int = None, override_tol: float = None); update the annotations to explicit union types using Python 3.10+ syntax (ppn: int | None, override_tol: float | None) so they satisfy PEP 484/Ruff RUF013 and leave the default values as None; no additional imports are necessary.
369-369: ImplicitOptionalindefine_case_dsignature (Ruff RUF013)
ppn: int = None,functor: Callable = None, andoverride_tol: float = Noneare all implicitOptional. Same PEP 484 concern as the__init__signature.♻️ Proposed fix
-def define_case_d(stack: CaseGeneratorStack, newTrace: str, newMods: dict, ppn: int = None, functor: Callable = None, override_tol: float = None, restart_check: bool = False) -> TestCaseBuilder: +def define_case_d(stack: CaseGeneratorStack, newTrace: str, newMods: dict, ppn: int | None = None, functor: Callable | None = None, override_tol: float | None = None, restart_check: bool = False) -> TestCaseBuilder:As per coding guidelines: "Python toolchain code. Follow PEP 8. Requires Python 3.10+."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` at line 369, The function signature for define_case_d uses implicit Optional types (ppn: int = None, functor: Callable = None, override_tol: float = None); update the annotations to explicit Optional types (ppn: Optional[int], functor: Optional[Callable[..., Any]] or Optional[Callable] per project conventions, override_tol: Optional[float]) and ensure Optional (and Any if used) are imported from typing at the top of the module so the signature matches PEP 484/PEP 8 requirements.
146-147: Add type annotations torun_restartfor consistency withrun
run(self, targets: List[Union[str, MFCTarget]], gpus: Set[int])is fully annotated;run_restartleavestargetsandgpusuntyped.♻️ Proposed fix
- def run_restart(self, targets, gpus): + def run_restart(self, targets: List[Union[str, MFCTarget]], gpus: Set[int]) -> subprocess.CompletedProcess:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` around lines 146 - 147, The run_restart method lacks type annotations; update its signature to match run by annotating targets and gpus as: def run_restart(self, targets: List[Union[str, MFCTarget]], gpus: Set[int]) (and add the same return type as run if present). Also ensure the module imports the typing symbols (List, Union, Set) and MFCTarget is in scope so the annotations resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@toolchain/mfc/test/case.py`:
- Line 155: The current assignment self.params = {**orig, 't_step_stop':
mid_step, 't_step_save': mid_step} incorrectly treats t_step_save as an absolute
step (assuming t_step_start == 0); change it to compute t_step_save relative to
the start offset by using t_step_save = mid_step - orig.get('t_step_start', 0)
so Phase 1 will produce a checkpoint exactly at mid_step even when orig contains
a nonzero 't_step_start'; update the dict construction using
orig.get('t_step_start', 0) and keep 't_step_stop': mid_step unchanged.
---
Nitpick comments:
In `@toolchain/mfc/test/case.py`:
- Around line 113-114: The __init__ signature in class Case (function __init__)
uses implicit Optional defaults (ppn: int = None, override_tol: float = None);
update the annotations to explicit union types using Python 3.10+ syntax (ppn:
int | None, override_tol: float | None) so they satisfy PEP 484/Ruff RUF013 and
leave the default values as None; no additional imports are necessary.
- Line 369: The function signature for define_case_d uses implicit Optional
types (ppn: int = None, functor: Callable = None, override_tol: float = None);
update the annotations to explicit Optional types (ppn: Optional[int], functor:
Optional[Callable[..., Any]] or Optional[Callable] per project conventions,
override_tol: Optional[float]) and ensure Optional (and Any if used) are
imported from typing at the top of the module so the signature matches PEP
484/PEP 8 requirements.
- Around line 146-147: The run_restart method lacks type annotations; update its
signature to match run by annotating targets and gpus as: def run_restart(self,
targets: List[Union[str, MFCTarget]], gpus: Set[int]) (and add the same return
type as run if present). Also ensure the module imports the typing symbols
(List, Union, Set) and MFCTarget is in scope so the annotations resolve
correctly.
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:L
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
toolchain/mfc/test/case.py (2)
114-114: Use explicit| Nonetype hints instead of implicitOptional.Ruff (RUF013) flags
ppn: int = Noneandoverride_tol: float = None— PEP 484 prohibits implicitOptional. Since the project requires Python 3.10+, the union syntax is available.♻️ Suggested fix
- def __init__(self, trace: str, mods: dict, ppn: int = None, override_tol: float = None, restart_check: bool = False) -> None: + def __init__(self, trace: str, mods: dict, ppn: int | None = None, override_tol: float | None = None, restart_check: bool = False) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` at line 114, The constructor signature for class __init__ uses implicit Optional via default None for parameters ppn and override_tol (ppn: int = None, override_tol: float = None); update the type hints to explicit union types using Python 3.10+ syntax (ppn: int | None and override_tol: float | None) in the __init__ definition so the annotations match PEP 484 and satisfy Ruff (RUF013) while leaving default values as None.
377-377: Same implicitOptionalissue ondefine_case_dparameters.Ruff (RUF013) flags the same pattern here. Apply the same
| Nonefix forppn,functor, andoverride_tol.♻️ Suggested fix
-def define_case_d(stack: CaseGeneratorStack, newTrace: str, newMods: dict, ppn: int = None, functor: Callable = None, override_tol: float = None, restart_check: bool = False) -> TestCaseBuilder: +def define_case_d(stack: CaseGeneratorStack, newTrace: str, newMods: dict, ppn: int | None = None, functor: Callable | None = None, override_tol: float | None = None, restart_check: bool = False) -> TestCaseBuilder:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` at line 377, The function define_case_d has implicit Optional types for ppn, functor, and override_tol; update the signature to make the optionality explicit by annotating ppn as int | None, functor as Callable | None, and override_tol as float | None (keeping the default = None), so the type hints match RUF013 expectations and align with the other fixes applied in this module.toolchain/mfc/test/cases.py (1)
1264-1286: Repeated 3D base setup could be extracted into a helper.The base-3D shock-tube construction (grid, BCs, geometry=9, patch centroids/lengths) is nearly identical in
restart_roundtrip_tests(Lines 1264–1286) andkernel_golden_tests(Lines 1301–1323), and similar inmpi_consistency_tests(Lines 1157–1180). A shared helper would reduce duplication.♻️ Sketch
def _make_base_3d(m=24, n=24, p=24): base = { 'm': m, 'n': n, 'p': p, 'x_domain%beg': 0.E+00, 'x_domain%end': 1.E+00, 'y_domain%beg': 0.E+00, 'y_domain%end': 1.E+00, 'z_domain%beg': 0.E+00, 'z_domain%end': 1.E+00, 'bc_x%beg': -3, 'bc_x%end': -3, 'bc_y%beg': -3, 'bc_y%end': -3, 'bc_z%beg': -3, 'bc_z%end': -3, } for pid in range(1, 4): base[f'patch_icpp({pid})%geometry'] = 9 for v in range(1, 4): base[f'patch_icpp({pid})%vel({v})'] = 0.0 base[f'patch_icpp({pid})%x_centroid'] = 0.5 base[f'patch_icpp({pid})%length_x'] = 1 base[f'patch_icpp({pid})%y_centroid'] = 0.5 base[f'patch_icpp({pid})%length_y'] = 1 base.update({ 'patch_icpp(1)%z_centroid': 0.05, 'patch_icpp(1)%length_z': 0.1, 'patch_icpp(2)%z_centroid': 0.45, 'patch_icpp(2)%length_z': 0.7, 'patch_icpp(3)%z_centroid': 0.9, 'patch_icpp(3)%length_z': 0.2, }) return baseAlso applies to: 1301-1323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/cases.py` around lines 1264 - 1286, Extract the duplicated 3D base setup into a helper function (e.g., _make_base_3d) and replace the repeated dict construction in restart_roundtrip_tests, kernel_golden_tests, and mpi_consistency_tests with calls to that helper; the helper should build the base dict (m,n,p, domain bounds, bc_*), loop patchID 1..3 to set 'patch_icpp({pid})%geometry'=9, vel(1..3)=0.0, x_centroid/length_x and y_centroid/length_y, and then update the three z_centroid/length_z entries before returning the dict so callers can further update or override values as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@toolchain/mfc/test/case.py`:
- Around line 146-183: The test omits POST_PROCESS when creating the straight
and restart outputs so D/ is empty and packer.pack() compares nothing; update
the test flow to ensure POST_PROCESS runs for both the initial straight run and
the restart execution: when invoking run() in the straight-path (calls at
test.py ~341) and inside run_restart (calls to self.run(...) and later result2 =
self.run([SIMULATION], gpus) in run_restart), include POST_PROCESS in the target
list (e.g., [PRE_PROCESS, SIMULATION, POST_PROCESS]) or call POST_PROCESS
separately after the restart completes so that D/ is populated before calling
packer.pack() and the tolerance comparison validates real output differences.
---
Duplicate comments:
In `@toolchain/mfc/test/case.py`:
- Line 155: The bug is that t_step_save is set to the absolute mid_step instead
of a save interval relative to t_step_start, so the MFC save condition
mod(t_step - t_step_start, t_step_save) == 0 won't fire when t_step_start>0; fix
by computing a relative interval (e.g. rel_save = mid_step -
orig.get('t_step_start', 0)), ensure rel_save is >=1 (fallback to 1), and set
self.params = {**orig, 't_step_stop': mid_step, 't_step_save': rel_save};
reference symbols: self.params, orig, mid_step, 't_step_start', 't_step_save',
't_step_stop'.
---
Nitpick comments:
In `@toolchain/mfc/test/case.py`:
- Line 114: The constructor signature for class __init__ uses implicit Optional
via default None for parameters ppn and override_tol (ppn: int = None,
override_tol: float = None); update the type hints to explicit union types using
Python 3.10+ syntax (ppn: int | None and override_tol: float | None) in the
__init__ definition so the annotations match PEP 484 and satisfy Ruff (RUF013)
while leaving default values as None.
- Line 377: The function define_case_d has implicit Optional types for ppn,
functor, and override_tol; update the signature to make the optionality explicit
by annotating ppn as int | None, functor as Callable | None, and override_tol as
float | None (keeping the default = None), so the type hints match RUF013
expectations and align with the other fixes applied in this module.
In `@toolchain/mfc/test/cases.py`:
- Around line 1264-1286: Extract the duplicated 3D base setup into a helper
function (e.g., _make_base_3d) and replace the repeated dict construction in
restart_roundtrip_tests, kernel_golden_tests, and mpi_consistency_tests with
calls to that helper; the helper should build the base dict (m,n,p, domain
bounds, bc_*), loop patchID 1..3 to set 'patch_icpp({pid})%geometry'=9,
vel(1..3)=0.0, x_centroid/length_x and y_centroid/length_y, and then update the
three z_centroid/length_z entries before returning the dict so callers can
further update or override values as needed.
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
2 similar comments
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
Contributor
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1213 +/- ##
==========================================
+ Coverage 44.05% 44.08% +0.02%
==========================================
Files 70 70
Lines 20496 20496
Branches 1989 1989
==========================================
+ Hits 9030 9035 +5
+ Misses 10328 10323 -5
Partials 1138 1138 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="toolchain/mfc/test/case.py">
<violation number="1" location="toolchain/mfc/test/case.py:184">
P2: Avoid swallowing all exceptions when recreating the test directory; it can mask filesystem failures and leave the test state inconsistent. Let the exception surface (or handle a specific expected error) so failures are visible.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
toolchain/mfc/test/case.py (3)
182-187: Silentexcept Exception: passin cleanup may hide disk-level failures.The intent (restore
case.pyon disk) is sound, but swallowing all exceptions makes it hard to diagnose failures in CI. Consider at least logging a warning so maintainers know the cleanup didn't succeed.Log the failure instead of silencing it
finally: self.params = orig try: self.create_directory() - except Exception: - pass + except Exception as exc: # noqa: BLE001 + cons.print(f"[yellow]Warning:[/yellow] Failed to restore case.py after restart: {exc}")(Requires importing
consfrom..printer.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` around lines 182 - 187, The finally block that restores self.params = orig and calls self.create_directory() currently swallows all exceptions with "except Exception: pass"; instead catch Exception as e and log a warning via the project's printer (import cons from ..printer) so disk-level failures are visible in CI; update the except on the create_directory call in case.py to use except Exception as e: cons.warn(...) (providing context like "failed to recreate case directory" and include e) rather than silently passing.
146-160: Phase 1 passes fulltargetslist includingPRE_PROCESS, while Phase 2 hardcodes[SIMULATION]— document the asymmetry.The design is intentional (Phase 1 needs grid generation; Phase 2 reuses the grid), but a reader unfamiliar with MFC's restart workflow may wonder why Phase 2 drops
PRE_PROCESS. A brief inline comment on line 171 would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` around lines 146 - 160, In run_restart, add a brief inline comment explaining why Phase 1 calls run with the full targets list (including PRE_PROCESS) while Phase 2 calls run with only [SIMULATION]: Phase 1 must generate the grid and restart data (hence PRE_PROCESS), and Phase 2 reuses that grid so only SIMULATION is required; place the comment near the two run(...) calls (referencing run_restart, run(targets, gpus), PRE_PROCESS and SIMULATION) so readers understand the intentional asymmetry.
114-114: PEP 484 prohibits implicitOptional; useT | None(Python 3.10+).Ruff flags
ppn: int = Noneandoverride_tol: float = Noneas implicit optionals. Since the toolchain targets Python 3.10+, the union syntax is available.Suggested fix
- def __init__(self, trace: str, mods: dict, ppn: int = None, override_tol: float = None, restart_check: bool = False) -> None: + def __init__(self, trace: str, mods: dict, ppn: int | None = None, override_tol: float | None = None, restart_check: bool = False) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/test/case.py` at line 114, The __init__ signature uses implicit Optional by assigning None to typed parameters (ppn: int = None, override_tol: float = None); update the annotations to explicit union types supported in Python 3.10+ (use ppn: int | None and override_tol: float | None) in the __init__ method to satisfy PEP 484/ruff; keep the default values as None and leave restart_check: bool unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@toolchain/mfc/test/test.py`:
- Line 395: The run_restart function currently accepts a targets parameter but
only uses it for Phase 1 while Phase 2 always uses [SIMULATION]; update the code
to avoid confusion by either (A) clarifying the behavior in run_restart's
docstring (explicitly state that the targets argument applies only to Phase 1
and Phase 2 is always [SIMULATION]) or (B) simplify the API by removing the
targets parameter from run_restart and hardcoding the intended phases (e.g.,
PRE_PROCESS then SIMULATION) inside the function; locate references to
run_restart and the Phase 2 hardcode to make the change and update any
callers/tests accordingly.
---
Nitpick comments:
In `@toolchain/mfc/test/case.py`:
- Around line 182-187: The finally block that restores self.params = orig and
calls self.create_directory() currently swallows all exceptions with "except
Exception: pass"; instead catch Exception as e and log a warning via the
project's printer (import cons from ..printer) so disk-level failures are
visible in CI; update the except on the create_directory call in case.py to use
except Exception as e: cons.warn(...) (providing context like "failed to
recreate case directory" and include e) rather than silently passing.
- Around line 146-160: In run_restart, add a brief inline comment explaining why
Phase 1 calls run with the full targets list (including PRE_PROCESS) while Phase
2 calls run with only [SIMULATION]: Phase 1 must generate the grid and restart
data (hence PRE_PROCESS), and Phase 2 reuses that grid so only SIMULATION is
required; place the comment near the two run(...) calls (referencing
run_restart, run(targets, gpus), PRE_PROCESS and SIMULATION) so readers
understand the intentional asymmetry.
- Line 114: The __init__ signature uses implicit Optional by assigning None to
typed parameters (ppn: int = None, override_tol: float = None); update the
annotations to explicit union types supported in Python 3.10+ (use ppn: int |
None and override_tol: float | None) in the __init__ method to satisfy PEP
484/ruff; keep the default values as None and leave restart_check: bool
unchanged.
…l tests Four new test categories to catch edge-case bugs: 1. Direction symmetry: 3D shock tube tests with shock propagating in x and y directions (default 3D tests use z), catching direction-specific bugs in reconstruction and gradient calculations. 2. MPI consistency: ppn=2 tests for bubbles, viscous flows, and hypoelasticity, catching broadcast/reduction bugs in MPI-sensitive physics modules. 3. Restart roundtrip: 1D and 3D tests that run a straight simulation, then restart from the midpoint and compare the restarted output against the straight run output to verify restart I/O fidelity. 4. Kernel golden values: 3D grid stretching test exercising non-uniform grid spacing in all three directions, which interacts with WENO reconstruction and gradient calculations direction-specifically. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oat consistency - Add timeout check after run_restart completes to catch hung restarts - Restore case.py on disk in finally block so test_all mode uses correct params - Use 1.0 instead of 1 for float consistency in direction symmetry tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add pylint disable comments for too-many-arguments/too-many-positional-arguments on TestCase.__init__ and too-many-instance-attributes on TestCaseBuilder, following the existing pattern already used in define_case_d/define_case_f. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mid_step = t_step_stop // 2 produced an invalid Phase 1 run when t_step_start > 0, since the midpoint could fall before the start step. Use (t_step_start + t_step_stop) // 2 so Phase 1 always has a valid range. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Restart roundtrip: Phase 2 now runs only SIMULATION (skips pre_process) since it reads grid+IC directly from p_all/p0/<mid_step>/. D/ is preserved across phases and intermediate step files are cleaned up so the final output matches a straight run. Grid stretching: Enlarge patches to cover the cosh-stretched domain (which expands beyond [0,1] to ~[0,1.39] with a=2, x_a=0.3, x_b=0.7). Without this, cells beyond the original bounds are uninitialized. Hypoelasticity MPI: Add dt=1e-06 matching other 3D hypoelasticity tests (default dt=0.0005 exceeds CFL for the high speed of sound ~88). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use mid_step - t_step_start instead of bare mid_step so the save interval aligns correctly when t_step_start > 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…oat consistency - Use common.delete_file instead of raw os.remove for intermediate step cleanup (prevents filesystem errors from crashing a passing test) - Guard finally block's create_directory so it cannot mask the original exception on failure - Use direct case.restart_check instead of getattr with default (fail loudly if attribute is removed rather than silently skipping tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sbryngelson
force-pushed
the
better-tests
branch
from
0108e49 to
ca20015
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Implementation Details
case.py: Addedrestart_checkflag toTestCaseandTestCaseBuilder, plusrun_restart()method that does a two-phase execution (run to midpoint, delete D/, restart from midpoint)cases.py: Four new test functions called after existing test generators, before sanity checkstest.py: After golden comparison succeeds, restart-flagged cases run the roundtrip check and compare against the straight-run packTest plan
list_cases())./mfc.sh test --generateto generate golden files for new cases./mfc.sh testto verify all new cases pass--mpiflag for the 3 ppn=2 consistency tests🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Data
CodeAnt-AI Description
Add direction-symmetry, MPI, restart roundtrip, and grid-stretching kernel tests
What Changed
Impact
✅ Fewer direction-specific reconstruction/gradient failures✅ Fewer MPI broadcast/reduction regressions during multi-rank runs✅ Clearer detection of restart I/O drift and restart regressions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.