Throw error in trixi_include when assignment is not found

[efaulhaber]

This gives more safety by throwing an error when an assignment to be replaced is passed to trixi_include and can't be found.

julia> trixi_include(default_example(), tspan=(0.0, 10.0))
ERROR: ArgumentError: assignment `tspan` not found in expression
Stacktrace:
   [1] find_assignment(expr::Expr, destination::Symbol)
     @ Trixi /mnt/ssd/home/efaulhab/git/Trixi.jl/src/auxiliary/special_elixirs.jl:272
   [2] trixi_include(mod::Module, elixir::String; kwargs::Base.Pairs{Symbol, Tuple{Float64, Float64}, …})
     @ Trixi /mnt/ssd/home/efaulhab/git/Trixi.jl/src/auxiliary/special_elixirs.jl:46
   [3] trixi_include
     @ /mnt/ssd/home/efaulhab/git/Trixi.jl/src/auxiliary/special_elixirs.jl:38 [inlined]
   [4] #trixi_include#1630
     @ /mnt/ssd/home/efaulhab/git/Trixi.jl/src/auxiliary/special_elixirs.jl:57 [inlined]

I implemented this for TrixiParticles, where it's really useful to catch errors not just when including elixirs from the REPL, but especially in the tests. It happened quite often that we changed a variable name in an elixir, but didn't notice that it was replaced in the tests with trixi_include, which then didn't work as expected. With this PR, it will just throw an error and fail.

[github-actions]

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

• The PR has a single goal that is clear from the PR title and/or description.
• All code changes represent a single set of modifications that logically belong together.
• No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

• The code can be understood easily.
• Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
• There are no redundancies that can be removed by simple modularization/refactoring.
• There are no leftover debug statements or commented code sections.
• The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

• New functions and types are documented with a docstring or top-level comment.
• Relevant publications are referenced in docstrings (see example for formatting).
• Inline comments are used to document longer or unusual code sections.
• Comments describe intent ("why?") and not just functionality ("what?").
• If the PR introduces a significant change or new feature, it is documented in NEWS.md.

Testing

• The PR passes all tests.
• New or modified lines of code are covered by tests.
• New or modified tests run in less then 10 seconds.

Performance

• There are no type instabilities or memory allocations in performance-critical parts.
• If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

• The correctness of the code was verified using appropriate tests.
• If new equations/methods are added, a convergence test has been run and the results
  are posted in the PR.

Created with ❤️ by the Trixi.jl community.

[sloede]

I like the idea. Have you checked if this has any measurable impact on run time?

[efaulhaber]

Not noticeable during normal usage:

main:

julia> @btime trixi_include(default_example(), sol=nothing, summary_callback=Nothing, callbacks=nothing);
[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.
[...]
[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.
  4.626 ms (6238 allocations: 3.46 MiB)

This branch:

julia> @btime trixi_include(default_example(), sol=nothing, summary_callback=Nothing, callbacks=nothing);
[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.
[...]
[ Info: You just called `trixi_include`. Julia may now compile the code, please be patient.
  5.725 ms (8136 allocations: 3.54 MiB)

Also, as you know, I'm a big fan of proper testing, so I added tests as well.

[sloede]

Please fix the formatting 🙂

[sloede]

Small modification, but other than that it LGTM! One look at the code part by @ranocha would be useful though

[ranocha]

Thanks a lot! I do like the idea 👍

[ranocha]

Looks like the insertion of maxiters failed?

  ArgumentError: assignment `maxiters` not found in expression
  Stacktrace:
    [1] find_assignment(expr::Expr, destination::Symbol)
      @ Trixi ~/work/Trixi.jl/Trixi.jl/src/auxiliary/special_elixirs.jl:272

See https://github.com/trixi-framework/Trixi.jl/actions/runs/6866385103/job/18672501258?pr=1737#step:8:1141

[efaulhaber]

Looks like the insertion of maxiters failed?

Yes, but I don't see why. It's working locally.

[efaulhaber]

Update: I can reproduce it now. When running the tests with coverage, maxiters=1 is inserted. This is why I didn't have this problem locally when running without coverage.

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (09bdf97) 96.28% compared to head (42138cd) 95.83%.

Files	Patch %	Lines
src/auxiliary/special_elixirs.jl	75.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1737      +/-   ##
==========================================
- Coverage   96.28%   95.83%   -0.45%     
==========================================
  Files         428      428              
  Lines       34621    34635      +14     
==========================================
- Hits        33333    33192     -141     
- Misses       1288     1443     +155     

Flag	Coverage Δ
unittests	95.83% <75.00%> (-0.45%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[ranocha]

Another real error:

plot 3D: Error During Test at /home/runner/work/Trixi.jl/Trixi.jl/test/test_trixi.jl:184
  Got exception outside of a @test
  ArgumentError: assignment `tspan` not found in expression
  Stacktrace:
    [1] find_assignment(expr::Expr, destination::Symbol)
      @ Trixi ~/work/Trixi.jl/Trixi.jl/src/auxiliary/special_elixirs.jl:272

at https://github.com/trixi-framework/Trixi.jl/actions/runs/6892797787/job/18750899415?pr=1737#step:7:2981

[ranocha]

Thanks a lot!

[sloede]

Does that mean I cannot use trixi_include for non Trixi files anymore? Ie, to include files that do not have a tspan variable etc? Because I have to admit, I liked the ability to include files and change variables also for non elixir files.

[ranocha]

As far as I understand, you can include whatever you like - you just get an error when you try to set a variable via a keyword argument of trixi_include that doesn't exist in the file you're including

[sloede]

Ah, ok. So it does not have a set of variables like tspan or maxiters or whatever that it expects to find and set automatically, and thus will complain if it is not a proper elixir? Then it's fine (actually great!)

[ranocha]

I agree 👍 There seems to be just some last fix to make CI pass, and we can merge this great contribution.

[efaulhaber]

So it does not have a set of variables like tspan or maxiters or whatever that it expects to find and set automatically, and thus will complain if it is not a proper elixir?

Only @test_trixi_include, which can be replaced by @test_nowarn_mod trixi_include for files that don't have maxiters.

[sloede]

So it does not have a set of variables like tspan or maxiters or whatever that it expects to find and set automatically, and thus will complain if it is not a proper elixir?

Only @test_trixi_include, which can be replaced by @test_nowarn_mod trixi_include for files that don't have maxiters.

Thanks for the explanation!

[efaulhaber]

There is always another failing test after I fixed the single failing one 🙄

[ranocha]

Thanks a lot!