Closed huiyuxie closed 1 month ago
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.
NEWS.md
.Created with :heart: by the Trixi.jl community.
General question here: Is it better to conduct unit testing while making changes, or to perform the tests after completing all the changes? Are there any good examples of testing from this repository? Thanks!
General question here: Is it better to conduct unit testing while making changes, or to perform the tests after completing all the changes? Are there any good examples of testing from this repository? Thanks!
When you're working on numerical fluxes, you can add some unit tests, e.g., to parts like
https://github.com/trixi-framework/Trixi.jl/blob/1745df43b6f4aa5801fb1bc29f4d328f37b42035/test/test_unit.jl#L633-L660
for the compressible Euler equations. Here, you could add additional tests using inputs with eltype(u) == Float32
.
We should also add some full integration tests once something works completely - maybe by adding one or two new example elixirs showing how to run simulations with Float32
.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 96.12%. Comparing base (
3b52a30
) to head (60836bb
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi @ranocha I have revised the code based on our discussion. Please review it once more, and if this version is satisfactory, I will apply similar revisions to the subsequent tasks.
For dual numbers, I have no idea whether it will be copied to the GPU and I think CUDA.jl does not directly support this type. If they are to be executed on GPUs, I will explore alternative methods (probably through struct
) to manage them.
For documentation, are you referring to a general overview that explains the purpose of using a function like oftype
, or to detailed documentation that points to each specific instance in the code?
@sloede Could you please have a look as well?
Maybe we can even provide a developer-only convenience function such as
isexactf0
withisexactf0(0.5) == true
andisexactf0(0.4) == false
such that people can easily check whether they need to writeconvert(RealT, 0.4)
or can just use0.5f0
?
I'm not sure it's worth writing something that can be achieved easily by
julia> 0.5 == 0.5f0
true
julia> 0.4 == 0.4f0
false
Maybe we can even provide a developer-only convenience function such as
isexactf0
withisexactf0(0.5) == true
andisexactf0(0.4) == false
such that people can easily check whether they need to writeconvert(RealT, 0.4)
or can just use0.5f0
?I'm not sure it's worth writing something that can be achieved easily by
julia> 0.5 == 0.5f0 true julia> 0.4 == 0.4f0 false
Fair enough. If it's properly documented somewhere, this should be sufficient.
Currently, I apply
convert
to integers case by case. But I think it is not friendly for later contributors to implement new functions (i.e., it is not something straightforward to think about).
If we want to have a simple rule that's easy to remember, we should just recommend using convert
, one
, zero
, oftype
etc. I'm not sure whether others havesimilar use cases, but convert
etc. is at least used quite often, e.g., in the SciML ecosystem:
https://github.com/SciML/OrdinaryDiffEq.jl/blob/438f34ba24bab5a2f6aa478850a267478cc53dea/src/perform_step/low_order_rk_perform_step.jl#L749C6-L749C28 https://github.com/SciML/OrdinaryDiffEq.jl/blob/438f34ba24bab5a2f6aa478850a267478cc53dea/src/tableaus/low_order_rk_tableaus.jl#L539-L582
I started some benchmarks locally to see whether there is any impact on our standard benchmark examples. I would like to check the results before merging this PR.
@ranocha Yes and you prefer to break the whole thing into small pieces and merge them step by step? I originally thought this PR is for all the files in src/equations. Please note that this PR depends on another small PR, which is not merged. If you test the type stability, it will fail.
Is the benchmark for precision or speed?
Please wait. The test data normally covers as large range of inputs as possible. Why is a single set of data tested here for each equation?
We don't have infinitely many resources, so we just added some unit tests and checked whether the implementations behave generally well by running more involved simulation setups as a whole.
Codecov favors my main branch PR here ;)
Please check and do not say something that confuses me. I'm more than sensitive to plagiarism or similar issues. No evidence, no assumptions, and no mentions, please.
Thanks!
I left a message in slack - Please check and collaborate as otherwise there will be more and more bugs in the long run.
Could you please run the formatter, e.g., like this?
import Pkg
Pkg.activate(temp = true)
Pkg.add(PackageSpec(name = "JuliaFormatter", version="1.0.45"))
using JuliaFormatter
format(["benchmark", "examples", "ext", "src", "test", "utils"])
(when executed in the main directory of your clone of Trixi.jl)
Please review again - already formatted and added 3D test
Sorry that I am extremely busy until next Wednesday (including the weekends) please wait and take your time :)
Please help me rerun CI as I have no rights to do so
Address parts of #591 and redo #1604.
Tasks: