Skip to content

Improve code quality and testing coverage#38

Merged
hakkelt merged 11 commits intokul-optec:masterfrom
hakkelt:code-quality-improvements
Apr 3, 2026
Merged

Improve code quality and testing coverage#38
hakkelt merged 11 commits intokul-optec:masterfrom
hakkelt:code-quality-improvements

Conversation

@hakkelt
Copy link
Copy Markdown
Collaborator

@hakkelt hakkelt commented Mar 26, 2026

This PR enhances code quality through formatting and refactoring:

  • Replaces the standard library Test framework with Test Items Framework
  • Improves test coverage a little
  • Adds JET tests and fixes type instabilities (a few operators got significantly faster!)
  • Introduces benchmarking script (GitHub Action is to be added in the next PR)
  • Replaces current formatter (JuliaFormatter) with Runic.jl, which is faster and less intrusive (e.g., no line length enforcement that caused most of the changes earlier)
  • Refactors some "monster" functions into multiple smaller ones
  • Relaxes, simplifies and unifies types in the signature is mul! functions and added calls to check function from src/utils.jl instead to enforce constraints. This function checks storage type (e.g throws error for GPU arrays), element type, and shape of both input and output. For example,
function mul!(y, P::Ax_mul_Bxt{L1,L2,C,D}, b) where {L1,L2,C,D} 
         mul!(P.bufA, P.A, b) 
         mul!(P.bufB, P.B, b) 
         return mul!(y, P.bufA, P.bufB')
end

becomes

 function mul!(y::AbstractArray, P::Ax_mul_Bxt, b::AbstractArray) 
     check(y, P, b) 
     mul!(P.bufA, P.A, b) 
     mul!(P.bufB, P.B, b) 
     return mul!(y, P.bufA, P.bufB') 
  end

Test Items Framework has the following advantages over the standard Test package:


I used GitHub Copilot heavily, so I also added agent instructions and skill files that helped with the work.

@hakkelt hakkelt force-pushed the code-quality-improvements branch 2 times, most recently from 42154ca to e336bfa Compare March 27, 2026 00:34
hakkelt added 3 commits March 27, 2026 01:39
- Refactor tests to @testitem/@testmodule with tags for parallel-ready execution
- Break down large test files into per-operator @testitem blocks
- Refactor subpackages (FFTWOperators, NFFTOperators, DSPOperators, WaveletOperators) to @testitem
- Complete JET coverage: all public API in all packages (IDFT, IRDFT, MIMOFilt, Axt_mul_Bx, Ax_mul_Bxt, MyLinOp)
- Fix MyLinOp: parameterize function fields with F/G<:Function (eliminates runtime dispatch)
- Fix Aqua persistent_tasks: broken=true for infrastructure-constrained environments
- Fix import issues in subpackage JET tests (AbstractOperators missing)
- 2777 tests pass, 5 broken (infrastructure), 0 failed
@hakkelt hakkelt force-pushed the code-quality-improvements branch from e336bfa to 6a5bca5 Compare March 27, 2026 00:39
@hakkelt hakkelt force-pushed the code-quality-improvements branch from 6a5bca5 to f677d84 Compare March 28, 2026 14:20
@hakkelt hakkelt force-pushed the code-quality-improvements branch from f677d84 to ffecdd5 Compare March 28, 2026 21:26
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 28, 2026

Codecov Report

❌ Patch coverage is 92.53859% with 174 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.28%. Comparing base (8637568) to head (57562d0).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
src/batching/SpreadingBatchOp.jl 85.59% 35 Missing ⚠️
src/combination_rules.jl 82.05% 21 Missing ⚠️
src/syntax.jl 71.01% 20 Missing ⚠️
src/calculus/Scale.jl 68.08% 15 Missing ⚠️
src/calculus/Jacobian.jl 81.39% 8 Missing ⚠️
src/calculus/Sum.jl 90.00% 8 Missing ⚠️
src/linearoperators/MatrixOp.jl 84.31% 8 Missing ⚠️
src/calculus/Compose.jl 95.91% 6 Missing ⚠️
src/properties.jl 91.66% 6 Missing ⚠️
src/calculus/BroadCast.jl 96.35% 5 Missing ⚠️
... and 21 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   87.60%   89.28%   +1.67%     
==========================================
  Files          45       45              
  Lines        3026     3267     +241     
==========================================
+ Hits         2651     2917     +266     
+ Misses        375      350      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hakkelt
Copy link
Copy Markdown
Collaborator Author

hakkelt commented Mar 28, 2026

Hi @nantonel and @lostella, this PR is ready to review. It's relatively large, sorry for that, and I especially regret having changed the formatting. On the other hand, if you look at the commits, I think it is mostly straightforward to understand what's changed and why.

What do you think of the suggested changes? This PR is primarily preparatory work for my next PR that adds GPU support (coming soon!). Nothing is essential for that from the suggested changes in this PR, so just let me know if you don't like any of them.

@lostella
Copy link
Copy Markdown
Member

@hakkelt thanks, taking a look. Also, thank you for pointing out copilot usage, and wrapping it up with a good summary nevertheless!

@lostella lostella self-requested a review March 29, 2026 07:22
@lostella
Copy link
Copy Markdown
Member

@hakkelt maybe one question: any specific reason for splitting operators into different packages, but now moving their tests back together to the root test folder?

Comment thread WaveletOperators/Project.toml Outdated

[targets]
test = ["Aqua", "Random", "RecursiveArrayTools", "Test"]
test = ["Aqua", "JET", "Random", "RecursiveArrayTools", "Test", "TestItemRunner", "TestItems"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these needed in this package?

Copy link
Copy Markdown
Collaborator Author

@hakkelt hakkelt Mar 30, 2026

Choose a reason for hiding this comment

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

No, they are no longer needed, thanks! I removed them.

@hakkelt
Copy link
Copy Markdown
Collaborator Author

hakkelt commented Mar 30, 2026

@hakkelt maybe one question: any specific reason for splitting operators into different packages, but now moving their tests back together to the root test folder?

Oh, yes, that's a solution for a problem I spent quite some time with. So the problem was the shared utils.jl script. I didn't want to duplicate this file in each subproject, so previously it was included from the root test folder, which was a bit awkward but worked with the standard test framework. TestItemRunner, however, uses a different approach for utility functions: they must be defined in a module loaded by the framework to ensure that test items are truly independent and that utility modules are loaded only once. Moreover, TestItemRunner scans the entire project for test items, not only the root test folder. So, calling @run_package_tests from the root test folder runs tests for subpackages, even if they are in a separate folder. There is a function, TestItemRunner.run_tests, that accepts the path to scan as its first argument, but it is not public.

Therefore, I could choose from the following options:

  • Revert to the standard test framework.
  • Duplicate test utility functions (test_op, test_NLop, and gradient_fd) in subpackages, and use the non-exported TestItemRunner.run_tests function.
  • Handle all test items as part of a unified test suite that contains all tests from the repo. This option still allows subproject-specific tests to be located in their respective folders, but it would be a bit misleading, since one would need to load the root "test" environment anyway.

Maybe I could extend the "Custom Operators" page in the documentation with a "Subpackages" section that explains why one would want to create a subpackage and how to test subpackages.

@hakkelt hakkelt force-pushed the code-quality-improvements branch from 348c244 to 1f99e1d Compare March 31, 2026 10:00
@hakkelt hakkelt force-pushed the code-quality-improvements branch from 1f99e1d to 57562d0 Compare March 31, 2026 10:49
@hakkelt
Copy link
Copy Markdown
Collaborator Author

hakkelt commented Mar 31, 2026

Well, that was strange: I pushed a single commit that removed the stale test dependencies from WaveletOperators (b34ee83), and then many JET tests failed. My guess is that the latest JET version was previously blocked, an older version was used by the tests, and the latest version is stricter. I don't think it is related to the changes in WaveletOperators' Project.toml, but it is rather caused by an independent event (e.g., one of the dependencies got a new version that no longer blocks the latest JET version).

Anyway, I fixed the failures. Some of them were real correctness issues (the test coverage still has room for improvement...), others were type instability issues.

@lostella: Just let me know if you like any changes to this PR or if further explanation would be helpful.

@lostella
Copy link
Copy Markdown
Member

lostella commented Apr 3, 2026

@hakkelt looks good! Feel free to merge

@hakkelt
Copy link
Copy Markdown
Collaborator Author

hakkelt commented Apr 3, 2026

Great, thanks for the review!

@hakkelt
Copy link
Copy Markdown
Collaborator Author

hakkelt commented Apr 3, 2026

@lostella: Actually, one more question: Is "squash and merge" a good strategy for this PR? It touches almost all files with over 8000 lines changed...

@lostella
Copy link
Copy Markdown
Member

lostella commented Apr 3, 2026

@hakkelt I don’t know, what could the problem with squashing be? The alternative is (I guess) to just enforce rebase-and-merge, assuming the commits are meaningful

@lostella
Copy link
Copy Markdown
Member

lostella commented Apr 3, 2026

@hakkelt enabled rebase-and-merge

@hakkelt
Copy link
Copy Markdown
Collaborator Author

hakkelt commented Apr 3, 2026

Thanks. For large PRs, like this one, rebase-and-merge is better.

@hakkelt hakkelt merged commit e4b47cf into kul-optec:master Apr 3, 2026
9 checks passed
@hakkelt hakkelt deleted the code-quality-improvements branch April 3, 2026 19:14
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.

2 participants