Skip to content

Conversation

@Cowsreal
Copy link
Contributor

@Cowsreal Cowsreal commented Nov 29, 2025

User description

User description

User description

PR just to run test suite to refresh coverage.


PR Type

Enhancement


Description

  • Removed unused s_derive_sound_speed subroutine and its public export

  • Removed unused s_solve_linear_system helper subroutine

  • Cleaned up module interface by eliminating dead code


Diagram Walkthrough

flowchart LR
  A["m_derived_variables module"] -- "remove unused subroutines" --> B["Streamlined module interface"]
  A -- "remove s_derive_sound_speed" --> C["Sound speed calculation removed"]
  A -- "remove s_solve_linear_system" --> D["Linear solver removed"]
Loading

File Walkthrough

Relevant files
Code cleanup
m_derived_variables.fpp
Remove unused sound speed and linear solver subroutines   

src/post_process/m_derived_variables.fpp

  • Removed s_derive_sound_speed from public interface exports
  • Deleted entire s_derive_sound_speed subroutine implementation (56
    lines)
  • Deleted entire s_solve_linear_system helper subroutine (46 lines)
  • Eliminated unused sound speed computation logic and linear system
    solver
+0/-103 


CodeAnt-AI Description

Drop unused sound-speed and solver helpers from derived variables

What Changed

  • Removed the exported sound-speed derivation routine so only the actively used derived-variable calculations remain
  • Deleted the private linear-system solver since no current workflow depends on it

Impact

✅ Leaner derived-variable exports
✅ Prevents accidental calls to obsolete helpers
✅ Keeps derived-variable maintenance focused on active calculations

💡 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:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

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:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

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

  • Refactor

    • Removed internal post-processing computational routines; public interface simplified while other derived-variable computations and flux limiter remain unchanged.
  • Tests

    • Added extra flux-related test configurations across dimensions and formats.
    • Added multiple golden metadata snapshots and expanded golden test data for deterministic test validation.

✏️ Tip: You can customize this high-level summary in your review settings.


PR Type

Enhancement, Tests


Description

  • Removed unused s_derive_sound_speed subroutine from public exports

  • Deleted unused s_derive_sound_speed and s_solve_linear_system implementations

  • Added flux_wrt=T test case to coverage testing suite

  • Streamlined module interface by eliminating 103 lines of dead code


Diagram Walkthrough

flowchart LR
  A["m_derived_variables module"] -- "remove unused subroutines" --> B["Streamlined interface"]
  A -- "delete s_derive_sound_speed" --> C["Sound speed removed"]
  A -- "delete s_solve_linear_system" --> D["Linear solver removed"]
  E["Test suite"] -- "add flux_wrt case" --> F["Enhanced coverage"]
Loading

File Walkthrough

Relevant files
Code cleanup
m_derived_variables.fpp
Remove unused sound speed and solver subroutines                 

src/post_process/m_derived_variables.fpp

  • Removed s_derive_sound_speed from public interface exports
  • Deleted entire s_derive_sound_speed subroutine implementation (56
    lines)
  • Deleted entire s_solve_linear_system helper subroutine (46 lines)
  • Eliminated unused sound speed computation and linear system solver
    logic
+0/-103 
Tests
cases.py
Add flux_wrt test case to coverage suite                                 

toolchain/mfc/test/cases.py

  • Added new test case with flux_wrt=T parameter to foreach_dimension()
    function
  • Inserted test case definition after alter_bc_patches() call
  • Expands test coverage for flux writer functionality
+3/-0     

@codeant-ai
Copy link

codeant-ai bot commented Nov 29, 2025

CodeAnt AI is reviewing your PR.

@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

Public interface still references only existing exports; verify no external calls remain to removed subroutines to avoid unresolved references at link/runtime.

   private; public :: s_initialize_derived_variables_module, &
s_derive_specific_heat_ratio, &
s_derive_liquid_stiffness, &
s_derive_flux_limiter, &
s_derive_vorticity_component, &
s_derive_qm, &
s_derive_liutex, &
API Removal

Removal of s_derive_sound_speed may break downstream code paths expecting sound speed; confirm alternative computation exists or update callers accordingly.

            do i = -offset_x%beg, m + offset_x%end
                q_sf(i, j, k) = pi_inf_sf(i, j, k)/(gamma_sf(i, j, k) + 1._wp)
            end do
        end do
    end do

end subroutine s_derive_liquid_stiffness

!>  This subroutine derives the flux_limiter at cell boundary

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 29, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Removed two subroutines (s_derive_sound_speed, s_solve_linear_system) and their public export from the m_derived_variables module; added a new flux-wrt test-case variant via alter_flux_wrt() in toolchain/mfc/test/cases.py; multiple golden metadata and test-data files were added.

Changes

Cohort / File(s) Summary
Module: removed subroutines
src/post_process/m_derived_variables.fpp
Deleted implementations of s_derive_sound_speed and s_solve_linear_system; removed s_derive_sound_speed from the module's public exports. Remaining module logic unchanged.
Test harness: new test-case variant
toolchain/mfc/test/cases.py
Added alter_flux_wrt() and invoked it inside foreach_dimension(), adding two Flux WRT test cases (default and format=2) per dimension.
Golden metadata (new files)
tests/*/golden-metadata.txt
Added multiple new golden-metadata snapshot files capturing environment/build/test metadata for different test runs (purely static data).
Golden test data (new files/entries)
tests/B0F13417/golden.txt, tests/C1A34E76/golden.txt, tests/.../D/cons*.dat, tests/.../D/prim*.dat
Added numerous golden data entries and files (cons..dat, prim..dat variants) used for test validation; content is static numeric datasets.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Check for any remaining references or calls to s_derive_sound_speed or s_solve_linear_system across the codebase (build errors or linker failures).
  • Confirm whether removed functionality needs replacement or documentation updates (sound-speed calculation, linear solver).
  • Validate new test cases in toolchain/mfc/test/cases.py run as expected and that golden files correspond to the intended outputs.

Possibly related PRs

Suggested labels

Review effort 3/5, size:M

Suggested reviewers

  • sbryngelson
  • wilfonba

Poem

I nibble lines of code with care,
Two old routines float in the air,
New tests hop in, a sprightly crew,
Golden files gleam like morning dew—
🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Code Coverage Testing' is vague and generic. It does not clearly convey the main change (removal of unused code) or specifically reference the primary modification to the codebase. Consider a more specific title such as 'Remove unused sound speed and linear solver subroutines from m_derived_variables' to better reflect the primary code cleanup changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The PR description covers the main changes and includes relevant details about the unused subroutines removed and test additions. However, it lacks specific sections matching the template (Type of change checklist, How Has This Been Tested section with test configuration, and many checklist items are unchecked).
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 15a4880 and 7bd189b.

📒 Files selected for processing (8)
  • tests/20E449F6/golden-metadata.txt (1 hunks)
  • tests/5363F3DE/golden-metadata.txt (1 hunks)
  • tests/800A7F9D/golden-metadata.txt (1 hunks)
  • tests/8F11D21E/golden-metadata.txt (1 hunks)
  • tests/B0F13417/golden-metadata.txt (1 hunks)
  • tests/B0F13417/golden.txt (1 hunks)
  • tests/C1A34E76/golden-metadata.txt (1 hunks)
  • tests/C1A34E76/golden.txt (1 hunks)
✅ Files skipped from review due to trivial changes (5)
  • tests/5363F3DE/golden-metadata.txt
  • tests/800A7F9D/golden-metadata.txt
  • tests/20E449F6/golden-metadata.txt
  • tests/C1A34E76/golden.txt
  • tests/8F11D21E/golden-metadata.txt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to **/*.{fpp,f90} : Name public subroutines with s_<verb>_<noun> pattern (e.g., s_compute_flux)
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification

Applied to files:

  • tests/B0F13417/golden-metadata.txt
  • tests/B0F13417/golden.txt
  • tests/C1A34E76/golden-metadata.txt
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers

Applied to files:

  • tests/B0F13417/golden-metadata.txt
  • tests/C1A34E76/golden-metadata.txt
📚 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: Compile with Cray `ftn` or NVIDIA `nvfortran` for GPU offloading; also build CPU-only with GNU `gfortran` and Intel `ifx`/`ifort` for portability

Applied to files:

  • tests/B0F13417/golden-metadata.txt
  • tests/C1A34E76/golden-metadata.txt
📚 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:

  • tests/B0F13417/golden-metadata.txt
  • tests/C1A34E76/golden-metadata.txt
⏰ 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). (11)
  • GitHub Check: Code Cleanliness Check
  • GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
  • GitHub Check: Github (macos, mpi, debug, false)
  • GitHub Check: Github (macos, mpi, no-debug, false)
  • GitHub Check: Github (ubuntu, mpi, no-debug, true)
  • GitHub Check: Github (ubuntu, mpi, no-debug, false)
  • GitHub Check: Github (ubuntu, mpi, debug, false)
  • GitHub Check: Github (ubuntu, mpi, debug, true)
  • GitHub Check: Coverage Test on CodeCov
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: Build & Publish
🔇 Additional comments (3)
tests/B0F13417/golden.txt (1)

1-16: Golden test data format unclear—verify "D/" prefix and data accuracy.

The numeric patterns in this golden file look physically reasonable (uniform regions with small perturbations in transition zones), but two clarifications would help ensure correctness:

  1. The "D/" prefix on each line is not self-documenting. Confirm this format aligns with the test framework's golden data convention and that it's handled correctly by the test harness.
  2. Verify these values are actual outputs from the new flux_wrt test run and not placeholder data. Given the PR's purpose is to "refresh coverage" by running the test suite, ensure the golden file captures deterministic, repeatable output.
tests/C1A34E76/golden-metadata.txt (1)

1-191: Comprehensive metadata snapshot; clarify if auto-generated.

This golden metadata file is well-structured and captures the full build/runtime environment (compiler versions, CMake flags, CPU hardware, Git state), which is valuable for reproducibility. However:

  1. Auto-generation: Is this file intended to be auto-generated by the test harness on each run? If so, it should probably not be manually committed; the test framework should handle generation and comparison.
  2. Non-deterministic values: Line 163 shows "CPU(s) scaling MHz: 81%", which may vary between runs. Confirm the test framework handles such variability correctly (ignores or normalizes).

If this file is meant to be a static golden reference, ensure all dynamic values (CPU frequency, timestamps) are either removed or masked during comparison.

tests/B0F13417/golden-metadata.txt (1)

1-191: Consistent metadata structure; confirm both test cases are distinct and necessary.

This file follows the same comprehensive structure as tests/C1A34E76/golden-metadata.txt and captures the test B0F13417 environment identically. The metadata is well-organized and complete.

However, with two similar golden-metadata files now in the PR, confirm:

  1. Both test cases (B0F13417 and C1A34E76) are distinct and necessary for the flux_wrt coverage goal.
  2. The metadata structure is stable and reproducible across runs (address the auto-generation and non-deterministic value concerns raised in the C1A34E76 review).

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 29, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@codeant-ai
Copy link

codeant-ai bot commented Nov 29, 2025

CodeAnt AI finished reviewing your PR.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

@Cowsreal Cowsreal closed this Nov 29, 2025
@Cowsreal Cowsreal reopened this Nov 29, 2025
@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

API Change

Public removal of s_derive_sound_speed may break callers; verify no remaining references and update any interfaces or docs accordingly.

   private; public :: s_initialize_derived_variables_module, &
s_derive_specific_heat_ratio, &
s_derive_liquid_stiffness, &
s_derive_flux_limiter, &
s_derive_vorticity_component, &
s_derive_qm, &
s_derive_liutex, &
Test Addition

New case with flux_wrt=T was added to the test enumerations; ensure corresponding validation or assertions exist so it exercises behavior rather than only enumerating cases.

cases.append(define_case_d(stack, "flux_wrt=T", {'flux_wrt': 'T'}))

stack.pop()

alter_mixlayer_perturb(dimInfo)
alter_bc_patches(dimInfo)

cases.append(define_case_d(stack, "flux_wrt=T", {'flux_wrt': 'T'}))
Copy link
Contributor

@qodo-merge-pro qodo-merge-pro bot Nov 29, 2025

Choose a reason for hiding this comment

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

Suggestion: Add a test case for flux_wrt = 'F' to complement the newly added test for flux_wrt = 'T', ensuring both branches of the logic are tested. [general, importance: 5]

Suggested change
cases.append(define_case_d(stack, "flux_wrt=T", {'flux_wrt': 'T'}))
cases.append(define_case_d(stack, "flux_wrt=T", {'flux_wrt': 'T'}))
cases.append(define_case_d(stack, "flux_wrt=F", {'flux_wrt': 'F'}))

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
toolchain/mfc/test/cases.py (1)

976-982: Consider adding a test case for flux_wrt='F'.

A past review comment suggested adding a test for flux_wrt='F' to complement the flux_wrt='T' case. This would ensure both branches of the logic are tested.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5596fdb and 15a4880.

📒 Files selected for processing (1)
  • toolchain/mfc/test/cases.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
toolchain/mfc/test/cases.py (1)
toolchain/mfc/test/case.py (3)
  • push (325-327)
  • define_case_d (339-355)
  • pop (329-330)
🪛 GitHub Actions: Lint Toolchain
toolchain/mfc/test/cases.py

[error] 978-978: Pylint: Trailing whitespace (C0303) detected. Lint step 'mfc.sh lint' failed with exit code 16.


[error] 980-980: Pylint: Trailing whitespace (C0303) detected. Lint step 'mfc.sh lint' failed with exit code 16.

⏰ 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). (11)
  • GitHub Check: Code Cleanliness Check
  • GitHub Check: Coverage Test on CodeCov
  • GitHub Check: Github (macos, mpi, no-debug, false)
  • GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
  • GitHub Check: Github (ubuntu, mpi, debug, true)
  • GitHub Check: Github (ubuntu, mpi, no-debug, false)
  • GitHub Check: Github (macos, mpi, debug, false)
  • GitHub Check: Github (ubuntu, mpi, no-debug, true)
  • GitHub Check: Github (ubuntu, mpi, debug, false)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: Build & Publish
🔇 Additional comments (1)
toolchain/mfc/test/cases.py (1)

1007-1007: LGTM!

The function is invoked appropriately within foreach_dimension(), following the established pattern for test case generation.

Comment on lines +976 to +982
def alter_flux_wrt():
stack.push("Flux WRT", {'flux_wrt': 'T'})

cases.append(define_case_d(stack, '', {}))
cases.append(define_case_d(stack, 'format=2', {'format': 2}))
stack.pop()

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix trailing whitespace to resolve pipeline failures.

Lines 978 and 980 contain trailing whitespace, causing the lint step to fail.

Apply this diff to remove the trailing whitespace:

 def alter_flux_wrt():
     stack.push("Flux WRT", {'flux_wrt': 'T'})
-        
+
     cases.append(define_case_d(stack, '', {}))
-    cases.append(define_case_d(stack, 'format=2', {'format': 2}))        
+    cases.append(define_case_d(stack, 'format=2', {'format': 2}))
     stack.pop()
📝 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 alter_flux_wrt():
stack.push("Flux WRT", {'flux_wrt': 'T'})
cases.append(define_case_d(stack, '', {}))
cases.append(define_case_d(stack, 'format=2', {'format': 2}))
stack.pop()
def alter_flux_wrt():
stack.push("Flux WRT", {'flux_wrt': 'T'})
cases.append(define_case_d(stack, '', {}))
cases.append(define_case_d(stack, 'format=2', {'format': 2}))
stack.pop()
🧰 Tools
🪛 GitHub Actions: Lint Toolchain

[error] 978-978: Pylint: Trailing whitespace (C0303) detected. Lint step 'mfc.sh lint' failed with exit code 16.


[error] 980-980: Pylint: Trailing whitespace (C0303) detected. Lint step 'mfc.sh lint' failed with exit code 16.

🤖 Prompt for AI Agents
In toolchain/mfc/test/cases.py around lines 976 to 982, there are trailing
spaces at the ends of lines 978 and 980 causing lint failures; open the file and
remove the trailing whitespace characters on those two lines (ensure there are
no extra spaces after the closing parentheses/quotes), save the file, and run
the linter/format step to confirm the pipeline passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

1 participant