test: comment out dtPi equations in GHG_rhs test

[lwJi]

Summary

• Temporarily comment out dtPi tensor from dtEvolVarlist in GHG_rhs.wl test
• Comment out the corresponding SetEQN for dtPi equations
• Update golden file to reflect the removal of dtPi-related generated code

Test plan

• Verify tests pass with wolframscript -f test/AllTests.wl
• Confirm golden file matches regenerated output

[claude]

PR Review: Comment Out dtPi Equations in GHG_rhs Test

Summary

This PR temporarily removes the dtPi tensor and its equations from the GHG_rhs test by commenting out the relevant code. The changes are clean and well-documented, with the golden file updated to reflect the expected output.

---

Code Quality ✅

Strengths:

• Clean, minimal changes that clearly express intent
• Proper use of Wolfram Language multiline comment syntax (* ... *)
• Formatting improvements make the code more readable with the multiline GridTensors call
• The PR description clearly documents what was changed

Minor Observations:

• The commented-out equation on line 85 is quite long (>400 chars). Consider adding a brief comment explaining why this is commented out, not just what. For example:

  (* TODO: dtPi equations temporarily disabled - [reason/issue reference] *)
  SetEQN[dtPi[a_, b_], ...]

---

Potential Issues 🟡

1. Missing Context: The PR doesn't explain why dtPi is being commented out

   • Is this a known bug being worked on?
   • Is this a temporary workaround?
   • Should there be a GitHub issue tracking the re-enabling of this?
   • Recommendation: Add a TODO comment with context or issue reference

2. Test Coverage Impact: By removing dtPi:

   • The test now only covers 2 of the 3 evolution variables in the GHG formulation (dtg and dtPhi, but not dtPi)
   • This reduces test coverage for a complex tensor equation involving Christoffel symbols, constraint violations, and source terms
   • Question: Are there other tests that cover similar equation patterns?

3. Golden File Regeneration: The PR shows ~2,455 lines removed from the golden file

   • This is a massive reduction (~95% of the file based on deletion count)
   • Recommendation: Verify the golden file was regenerated correctly with ./test/run_tests.sh --generate
   • The test checklist mentions running wolframscript -f test/AllTests.wl but doesn't explicitly confirm golden file regeneration

---

Performance Considerations ✅

• Reducing generated code by ~2,455 lines will improve:
  • Compilation time
  • Test execution time
  • Code generation performance
• No performance concerns introduced by this change

---

Security Concerns ✅

• No security issues
• This is test code modification only

---

Testing 🟡

From PR Description:

• Verify tests pass with wolframscript -f test/AllTests.wl
• Confirm golden file matches regenerated output

Missing Test Evidence:

• The PR doesn't show test results
• Recommendation: Add a comment with test output showing:

  $ wolframscript -f test/AllTests.wl
  [test results showing all tests pass]

Test Coverage Questions:

1. Does this test still provide meaningful coverage of the GHG formulation with dtPi removed?
2. Are there integration tests that still verify the full system with all three evolution variables?
3. Should there be a separate, simpler test case added to replace this coverage?

---

Architecture & Maintenance Concerns ⚠️

1. Incomplete Test: This test is now incomplete as it represents a partial implementation of the GHG formulation

   • Original test had: dtg, dtPi, dtPhi
   • Current test has: dtg, dtPhi (missing dtPi)
   • The file name GHG_rhs.wl suggests this should be the full RHS computation

2. Golden File Validity:

   • The golden file test/golden/Nmesh/GHG_rhs.c.golden:19-21 still references dtPi variables in comments or initialization code
   • Action Required: Verify the golden file is consistent (no orphaned dtPi references)

3. Documentation:

   • The test/Nmesh/GHG_rhs.ipynb notebook likely still documents the full system with dtPi
   • Recommendation: Update the notebook to note that dtPi is temporarily disabled, or create a separate test file for the partial implementation

---

Recommendations

Before Merging:

1. ✅ Add a TODO comment explaining why dtPi is commented out (with issue reference if applicable)
2. ✅ Provide test output confirming all tests pass
3. ✅ Verify golden file has no orphaned dtPi references
4. 🟡 Consider creating a GitHub issue to track re-enabling dtPi (if not already exists)
5. 🟡 Update documentation (notebook) to reflect the temporary change

For Future:

1. Consider whether this test should be renamed to indicate it's a partial implementation (e.g., GHG_rhs_partial.wl)
2. Evaluate if a simpler test case would be better for CI rather than commenting out parts of a complex test
3. Add integration tests that verify the full GHG system if they don't exist

---

Verdict

Status: Approve with Minor Changes Requested ⚠️

The code changes are clean and the approach is reasonable for a temporary workaround. However, the PR would benefit from:

• Context on why this is being done
• Test evidence showing the change works
• Verification that the golden file is fully consistent

This appears to be a work-in-progress or temporary state. If this is intended to be permanent, I'd recommend a different approach (separate test file or removing the test entirely rather than commenting out portions).

---

Questions for the Author:

1. What issue necessitated commenting out the dtPi equations?
2. Is there a timeline for re-enabling this functionality?
3. Should we create a GitHub issue to track this?
4. Have you verified the golden file was regenerated with ./test/run_tests.sh --generate?
5. Does the GHG_rhs.ipynb notebook need updates to reflect this change?