Skip to content

TEST: Fix existing tests#64

Merged
oyamad merged 7 commits intoQuantEcon:mainfrom
mnshkw:test/fix-existing-tests
Mar 6, 2026
Merged

TEST: Fix existing tests#64
oyamad merged 7 commits intoQuantEcon:mainfrom
mnshkw:test/fix-existing-tests

Conversation

@mnshkw
Copy link
Collaborator

@mnshkw mnshkw commented Mar 5, 2026

Summary

Changes

test/runtests.jl

  • ea92243:
    • Add LQ to the import list from QuantEcon explicitly

test/test_cdp.jl

  • ea92243:

    • Remove using QuantEcon from inside the @testset block due to namespace pollution
    • Replace QUantEcon.LQ(...) with LQ(...)
  • 109efdb:

    • Remove s_star and x_star (dead code), unused variable sigma = 0.1, and replace hardcoded number to already-defined variable
  • 9b24891 :

    • Add checks for initial value, path length, and full path (previously, only the final step was checked against the analytical path)
    • Replace the single-step isapprox check with a full-path element-wise max absolute error check using maximum(abs, ...) <= atol
    • Relax atol from 1e-5 to 1e-4 to account for interpolation error accumulated in simulate
      • The observed maximum absolute error across all four basis × method combinations was ~1.68e-5, which exceeds 1e-5 but is well within 1e-4
      • The tolerance 1e-4 provides a safety margin while remaining a meaningful numerical bound
  • 4bbfe33:

    • Wrap the inner loop body in @testset "Test $method with $label basis" so that failures clearly identify which bases x method combination failed

Test Results

All tests are pass successfully (Pkg.test() passes locally).

Notes for Reviewers

All codes and documentation are written with assistance by LLMs

cc: @oyamad
Could you please review this PR? Thank you.

mnshkw added 4 commits March 4, 2026 05:00
…ngth, and full-path check

- change norm check to element-wise max abs error check
- change atol from 1e-5 to 1e-4 to avoid test failure due to numerical precision issues
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes and cleans up existing tests in test/test_cdp.jl and test/runtests.jl. It addresses namespace pollution from using QuantEcon inside a @testset block, removes dead code, replaces hardcoded values with named variables, improves simulation path assertions, and wraps inner test loops in labeled @testset blocks for better failure diagnostics.

Changes:

  • Move LQ import to runtests.jl's top-level using QuantEcon line and remove the inner using QuantEcon from the "LQ control" testset to avoid namespace pollution
  • Enhance simulation tests: add checks for initial value and path length, replace single-endpoint check with full-path max absolute error check, and relax atol from 1e-5 to 1e-4 to account for interpolation error accumulation
  • Clean up dead code (unused sigma, bare s_star, x_star expression) and replace hardcoded 3 with n_shocks variable; wrap inner loop body in labeled @testset blocks

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/runtests.jl Add LQ to the explicit import list from QuantEcon
test/test_cdp.jl Remove inner using QuantEcon, use bare LQ(...), add @testset labels, improve simulation assertions, remove dead code, use named variables

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

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

@oyamad oyamad merged commit 06b30aa into QuantEcon:main Mar 6, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants