Add example case for convergence test in 1D #1030
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-code-review[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR removes support for the pi constant in analytical patch expressions and adds a new 1D convergence example that demonstrates spatial accuracy testing. The removal of pi from the rhs_replace dictionary in the case parser is problematic because the new example and existing examples use pi in analytical expressions.
- Removes
piconstant support from the analytical patch expression parser - Adds a 1D two-component advection convergence test example with automation scripts
- Provides plotting utilities for analyzing convergence rates
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/case.py | Removes pi from the rhs_replace dictionary that maps variable names in analytical expressions |
| examples/1D_convergence/submitJobs.sh | Adds shell script to automate running convergence tests across multiple grid resolutions and WENO orders |
| examples/1D_convergence/plot.py | Adds Python script to compute and visualize L1, L2, and Linf error norms |
| examples/1D_convergence/case.py | Adds case file for 1D two-fluid advection test with analytical initial conditions using sinusoidal patterns |
| examples/1D_convergence/README.md | Adds documentation explaining how to run the convergence test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1030 +/- ##
=======================================
Coverage 44.36% 44.36%
=======================================
Files 71 71
Lines 20590 20590
Branches 1994 1994
=======================================
Hits 9134 9134
Misses 10310 10310
Partials 1146 1146 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
CodeAnt AI is reviewing your PR. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new 1D convergence example (README, config generator, plotting, job script); removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
Comment |
codeant-ai
bot
added
the
size:L
![codeant-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
toolchain/mfc/test/cases.py (1)
1005-1029: Fix malformedcasesToSkiplist causing IndentationErrorThe addition of
"1D_convergence"broke thecasesToSkiplist: line 1027 closes the list (",]"), and line 1028 is now a stray string literal, which matches the coverage/lintIndentationErrorat Line 1028.Update the list so both
"1D_convergence"and"3D_IGR_33jet"are inside the same list literal:- "2D_backward_facing_step", - "2D_forward_facing_step", - "1D_convergence",] - "3D_IGR_33jet"] + "2D_backward_facing_step", + "2D_forward_facing_step", + "1D_convergence", + "3D_IGR_33jet"]This restores valid syntax and keeps the intended examples skipped from testing.
toolchain/mfc/case.py (1)
135-147: I apologize—the repository clone failed, preventing direct verification. However, I can assess the review comment based on the code snippet provided and logical analysis.Let me verify the core logical concern:
<function_calls>
Simulate the issue based on the provided code snippet
python3 << 'EOF'
import re
import mathCurrent rhs_replace from the review (lines 135-147)
def rhs_replace_current(match):
return {
'x': 'x_cc(i)', 'y': 'y_cc(j)', 'z': 'z_cc(k)',
'xc': 'patch_icpp(pid)%x_centroid', 'yc': 'patch_icpp(pid)%y_centroid', 'zc': 'patch_icpp(pid)%z_centroid',
'lx': 'patch_icpp(pid)%length_x', 'ly': 'patch_icpp(pid)%length_y', 'lz': 'patch_icpp(pid)%length_z',
'r': 'patch_icpp(pid)%radius', 'eps': 'patch_icpp(pid)%epsilon', 'beta': 'patch_icpp(pid)%beta',
'tau_e': 'patch_icpp(pid)%tau_e', 'radii': 'patch_icpp(pid)%radii',
'e' : f'{math.e}',
# NOTE: 'pi' is ABSENT
}.get(match.group(), match.group())Test case: analytical expression with pi (as mentioned in review)
test_expr = "sin(2pix)"
print("ISSUE VERIFICATION:")
print("=" * 70)
print(f"\nTest analytical expression: {test_expr}")
print(f"Token replacement regex: r'[a-zA-Z]+'")Show tokens
tokens = re.findall(r"[a-zA-Z]+", test_expr)
print(f"Tokens found: {tokens}")Apply transformation
result = re.sub(r"[a-zA-Z]+", rhs_replace_current, test_expr)
print(f"\nGenerated Fortran (CURRENT): {result}")Show what would happen with pi mapping
def rhs_replace_fixed(match):
d = {
'x': 'x_cc(i)', 'y': 'y_cc(j)', 'z': 'z_cc(k)',
'xc': 'patch_icpp(pid)%x_centroid', 'yc': 'patch_icpp(pid)%y_centroid', 'zc': 'patch_icpp(pid)%z_centroid',
'lx': 'patch_icpp(pid)%length_x', 'ly': 'patch_icpp(pid)%length_y', 'lz': 'patch_icpp(pid)%length_z',
'r': 'patch_icpp(pid)%radius', 'eps': 'patch_icpp(pid)%epsilon', 'beta': 'patch_icpp(pid)%beta',
'tau_e': 'patch_icpp(pid)%tau_e', 'radii': 'patch_icpp(pid)%radii',
'e' : f'{math.e}',
'pi': f'{math.pi}', # ADDED
}
return d.get(match.group(), match.group())result_fixed = re.sub(r"[a-zA-Z]+", rhs_replace_fixed, test_expr)
print(f"Generated Fortran (WITH 'pi' mapping): {result_fixed}")print("\n" + "=" * 70)
print("CONCLUSION:")
print("- Current code leaves 'pi' unsubstituted in generated Fortran")
print("- This will cause undefined identifier error UNLESS Fortran defines 'pi'")
print("- The 'e' mapping is present, so asymmetry is notable")
print("- Risk: existing analytical examples may break or behave unexpectedly")EOF
</function_calls>The logic in the review comment is sound and verifiable from the provided code snippet alone:
- Confirmed from snippet: Lines 135–147 show
rhs_replacedictionary with'e': f'{math.e}'but no'pi'entry- Logical consequence: The regex
re.sub(r"[a-zA-Z]+", rhs_replace, expr)will leavepitokens unchanged when not in the mapping (defaults to.get(match.group(), match.group()))- Real impact: Analytical expressions like
sin(2*pi*x)referenced in the review will generate Fortran with barepiidentifiers instead of numeric constantsSince I cannot directly access the repository to verify:
- Whether
examples/1D_convergence/case.pyactually usespi- Whether Fortran modules define
piglobally- Git history of this change
I must rely on the review's logical assessment. The concern is inherently valid based on code analysis: removing
'pi'from token mapping while retaining'e'creates an asymmetry and a potential breaking change for analytical ICs.
Restore the
'pi'mapping inrhs_replaceto fix analytical expression token substitutionThe
rhs_replacefunction at lines 135–147 omits the'pi'entry while retaining'e'. Analytical initial conditions containingpi(e.g.,sin(2*pi*x)) will generate Fortran code with unsubstitutedpiidentifiers. Unless the Fortran module explicitly definespiin scope, this causes undefined identifier errors at compile-time.Add the mapping before the closing brace:
'pi': f'{math.pi}',Verify that existing analytical examples (particularly those with
piin expressions) still compile and converge as expected.
♻️ Duplicate comments (1)
examples/1D_convergence/plot.py (1)
63-69: Consider exporting L1 and Linf norms to CSV as wellRight now
errors.csvonly contains the L2 error for each order plus reference rates (R1,R3,R5). Since you’re already computing L1 and Linf and the README mentions all three norms, it may be useful to include them in the CSV for downstream analysis.One possible refactor (replacing the current block):
- errors = np.column_stack((N, errors[:, :, 0])) - errors = np.column_stack((errors, 7 / np.array(N) ** 1)) - errors = np.column_stack((errors, 700 / np.array(N) ** 3)) - errors = np.column_stack((errors, 2000 / np.array(N) ** 5)) - - df = pd.DataFrame(errors, columns=["N", "1", "3", "5", "R1", "R3", "R5"], index=N) - df.to_csv("errors.csv", index=False) + df_data = {"N": N} + for j, order in enumerate(Ord): + df_data[f"L2_O{order}"] = errors[:, j, 0] + df_data[f"L1_O{order}"] = errors[:, j, 1] + df_data[f"Linf_O{order}"] = errors[:, j, 2] + + # Optional reference lines, if you still want them in the CSV: + df_data["R1"] = 7 / np.array(N) ** 1 + df_data["R3"] = 700 / np.array(N) ** 3 + df_data["R5"] = 2000 / np.array(N) ** 5 + + df = pd.DataFrame(df_data) + df.to_csv("errors.csv", index=False)This keeps the reference columns and adds all three norms per order.
🧹 Nitpick comments (3)
examples/1D_convergence/case.py (2)
20-38: Minor cleanups forNthandling
Ntis already an integer; the extra cast int_step_stopis redundant:- "t_step_stop": int(Nt), + "t_step_stop": Nt,This keeps behavior identical and addresses the “unnecessary int()” warning.
70-73: Drop unnecessary f-strings for analytic IC expressionsThese four values are plain string literals; the
fprefix is unused and can be removed for clarity:- "patch_icpp(1)%alpha_rho(1)": f"0.5 - 0.5*sin(2*pi*x)", - "patch_icpp(1)%alpha(1)": f"0.5 - 0.5*sin(2*pi*x)", - "patch_icpp(1)%alpha_rho(2)": f"0.5 + 0.5*sin(2*pi*x)", - "patch_icpp(1)%alpha(2)": f"0.5 + 0.5*sin(2*pi*x)", + "patch_icpp(1)%alpha_rho(1)": "0.5 - 0.5*sin(2*pi*x)", + "patch_icpp(1)%alpha(1)": "0.5 - 0.5*sin(2*pi*x)", + "patch_icpp(1)%alpha_rho(2)": "0.5 + 0.5*sin(2*pi*x)", + "patch_icpp(1)%alpha(2)": "0.5 + 0.5*sin(2*pi*x)",No functional change, just style/clarity.
examples/1D_convergence/README.md (1)
1-11: Minor grammar tweak: remove extra commaLine 5 has an extra comma after “5th”:
3rd, and 5th, order spatial reconstructions.Recommend:
-3rd, and 5th, order spatial reconstructions. These settings can be modified by editing the variables +3rd, and 5th order spatial reconstructions. These settings can be modified by editing the variablesPurely cosmetic, improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/1D_convergence/README.md(1 hunks)examples/1D_convergence/case.py(1 hunks)examples/1D_convergence/plot.py(1 hunks)examples/1D_convergence/submitJobs.sh(1 hunks)toolchain/mfc/case.py(1 hunks)toolchain/mfc/test/cases.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Draft a step-by-step plan before making changes; build after each step using `./mfc.sh build -t pre_process simulation -j $(nproc)`
Applied to files:
examples/1D_convergence/submitJobs.sh
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: After each successful build step, run focused tests using `./mfc.sh test -j $(nproc) -f EA8FA07E -t 9E2CA336` instead of running all tests
Applied to files:
examples/1D_convergence/submitJobs.sh
🪛 GitHub Actions: Coverage Check
toolchain/mfc/test/cases.py
[error] 1028-1028: IndentationError: unexpected indent in toolchain/mfc/test/cases.py:1028.
🪛 GitHub Actions: Lint Toolchain
toolchain/mfc/test/cases.py
[error] 1028-1028: pylint: E0001: Parsing failed: 'unexpected indent (mfc.test.cases, line 1028)'
🪛 Ruff (0.14.6)
examples/1D_convergence/case.py
38-38: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
70-70: f-string without any placeholders
Remove extraneous f prefix
(F541)
71-71: f-string without any placeholders
Remove extraneous f prefix
(F541)
72-72: f-string without any placeholders
Remove extraneous f prefix
(F541)
73-73: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Shellcheck (0.11.0)
examples/1D_convergence/submitJobs.sh
[warning] 2-2: In POSIX sh, arrays are undefined.
(SC3030)
[warning] 3-3: In POSIX sh, arrays are undefined.
(SC3030)
[warning] 9-9: In POSIX sh, BASH_SOURCE is undefined.
(SC3028)
[warning] 9-9: In POSIX sh, array references are undefined.
(SC3054)
[warning] 9-9: In POSIX sh, &> is undefined.
(SC3020)
[warning] 13-13: In POSIX sh, array references are undefined.
(SC3054)
[warning] 14-14: In POSIX sh, array references are undefined.
(SC3054)
[warning] 21-21: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 23-23: In POSIX sh, array references are undefined.
(SC3054)
[warning] 24-24: In POSIX sh, array references are undefined.
(SC3054)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Publish
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![cubic-dev-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="examples/1D_convergence/submitJobs.sh">
<violation number="1" location="examples/1D_convergence/submitJobs.sh:1">
Script declares an sh shebang but uses Bash-only features (arrays and ${BASH_SOURCE[0]}), so it will fail under /bin/sh. Use a Bash interpreter.</violation>
</file>
<file name="toolchain/mfc/test/cases.py">
<violation number="1" location="toolchain/mfc/test/cases.py:1027">
The added "1D_convergence" entry closes the `casesToSkip` list with `]`, leaving the next string literal outside the list and causing a SyntaxError. Drop the bracket so the list stays well-formed.</violation>
</file>
<file name="examples/1D_convergence/README.md">
<violation number="1" location="examples/1D_convergence/README.md:2">
Running `./submitJobs.sh` as documented fails because the script’s shebang is `/usr/bin/env sh` but the script is written for bash-only syntax. Instruct users to invoke it with bash so it runs successfully.</violation>
</file>
<file name="examples/1D_convergence/case.py">
<violation number="1" location="examples/1D_convergence/case.py:17">
The -N flag is described as the desired grid-point count, but the script subtracts one before writing `m`, so each simulation runs with N-1 cells and the intended convergence levels (32, 64, …) are never realized.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="examples/1D_convergence/submitJobs.sh">
<violation number="1" location="examples/1D_convergence/submitJobs.sh:1">
The script is declared as an sh script but uses bash-only arrays, `${BASH_SOURCE[0]}`, and `&>` redirection, so it fails as soon as `/bin/sh` runs it. Update the shebang to bash so it runs successfully.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="examples/1D_convergence/submitJobs.sh">
<violation number="1" location="examples/1D_convergence/submitJobs.sh:17">
Relative file operations ignore ROOT_DIR, so running the script from outside the example folder fails to find case.py and creates N*_O* directories in the wrong place.</violation>
</file>
<file name="examples/1D_convergence/case.py">
<violation number="1" location="examples/1D_convergence/case.py:17">
The -N argument is documented as the number of grid points, but the script subtracts one before assigning `m`, so every simulation runs with one fewer point than requested.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
1 issue found across 1 file (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="examples/1D_convergence/submitJobs.sh">
<violation number="1" location="examples/1D_convergence/submitJobs.sh:20">
Copying case.py uses a relative path, so running the script outside its directory fails. Reference the computed ROOT_DIR when copying the template.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
2 participants
User description
User description
Description
This PR adds an example case with automated scripts to create convergence test results for a 1D advection case. It also fixes a toolchain error that resulted in reduced convergence when
piwas used for analaytic patches.Type of change
Scope
How Has This Been Tested?
oldConvergence.pdf
PR Type
Enhancement, Bug fix
Description
Add 1D convergence test example with automated scripts
Fix toolchain error with pi constant in analytic patches
Provide documentation and setup instructions for convergence testing
Diagram Walkthrough
File Walkthrough
case.py
1D convergence test case configuration generatorexamples/1D_convergence/case.py
convergence simulation
Riemann solver, and grid resolution
with sinusoidal density/volume fraction profiles
plot.py
Convergence error analysis and visualization toolexamples/1D_convergence/plot.py
grid resolutions and WENO orders
initial conditions
1, 3, and 5
submitJobs.sh
Automated job submission script for convergence testsexamples/1D_convergence/submitJobs.sh
orders (1, 3, 5)
appropriate parameters
README.md
Convergence test example documentation and setup guideexamples/1D_convergence/README.md
and usage
configuring paths
script
plots
case.py
Fix pi constant handling in analytic patchestoolchain/mfc/case.py
expressions
reduction
case definitions
Note
Adds a 1D two-fluid convergence example with job/plot scripts and README, removes hardcoded pi substitution in analytic patches, and skips the new example in tests.
examples/1D_convergence: New example withcase.py(config generator),submitJobs.sh(automation over N and WENO order),plot.py(L1/L2/Linf analysis and CSV export), andREADME.md(usage).toolchain/mfc/case.py: Removepiconstant substitution from analytic expression codegen (retaine).toolchain/mfc/test/cases.py: Add"1D_convergence"to skipped examples list.Written by Cursor Bugbot for commit bb4d52c. Configure here.
CodeAnt-AI Description
Add runnable 1D convergence example and keep analytic accuracy
What Changed
Impact
✅ Fewer manual convergence setups✅ Clearer convergence error plots✅ More accurate analytic patch evaluations💡 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.
Summary by CodeRabbit
New Features
Documentation
Tools
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.