Add numerical support of other real types (continue)

huiyuxie commented 1 month ago

Continue #1909.

Tasks:

[x] compressible_euler_multicomponent_1d, compressible_euler_multicomponent_2d
[x] compressible_euler_quasi_1d
[x] unit tests for above

github-actions[bot] commented 1 month ago

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 with its PR number.

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 :heart: by the Trixi.jl community.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.16%. Comparing base (c090422) to head (71b7a64).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1947 +/- ## ========================================== + Coverage 96.14% 96.16% +0.02% ========================================== Files 460 460 Lines 36926 36950 +24 ========================================== + Hits 35499 35530 +31 + Misses 1427 1420 -7 ``` | [Flag](https://app.codecov.io/gh/trixi-framework/Trixi.jl/pull/1947/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trixi-framework) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/trixi-framework/Trixi.jl/pull/1947/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trixi-framework) | `96.16% <100.00%> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trixi-framework#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

huiyuxie commented 1 month ago

Why do CI tests seem so vulnerable here?

CI tests could be easily passed locally https://github.com/huiyuxie/Trixi.jl/actions/runs/9135929568 and it seems like there is something wrong with the token in the upstream.

ranocha commented 1 month ago

Why do CI tests seem so vulnerable here?

CI tests could be easily passed locally https://github.com/huiyuxie/Trixi.jl/actions/runs/9135929568 and it seems like there is something wrong with the token in the upstream.

It's a Codecov issue for PRs from forks to open-source repositories, see https://github.com/trixi-framework/Trixi.jl/issues/1905 and https://github.com/codecov/feedback/issues/301

huiyuxie commented 1 month ago

@ranocha Please check the new push in the first PR (sample test).

The small amount of test data seems odd to me or maybe we can opt for a random data generator to feed the functions? I have tested the modified parts, but should I test all the functions even if they were not touched? BTW, the formatter looks bad to me, the code looks even worse after formatting - someone has to adjust the parameters for the current formatter.

Could you please provide your benchmark results for Float64 and Float32 if available? Does this running process default to multithreading? I also want to see your running environment (i.e., basic CPU information), if you would like to share.

huiyuxie commented 1 month ago

That PR has too many comments so I comment here.

ranocha commented 1 month ago

It starts to become messy if we discuss stuff about another PR here, so take everything with a grain of salt since I may be confused.

The small amount of test data seems odd to me or maybe we can opt for a random data generator to feed the functions?

I would like to avoid randomness (or at least print the values that are used for randomized testing so that debugging becomes possible).

I have tested the modified parts, but should I test all the functions even if they were not touched?

At least the modified parts should be tested. You can also use a test-driven workflow and start adding new tests (@test_broken) for functions that you will fix later in other PRs.

ranocha commented 4 weeks ago

Hi @huiyuxie! Thanks for your contributions. We have merged your other PR setting up the testing infrastructure. Thus, you can go ahead and add tests in this PR as well.

Please also add your name to our list of authors in https://github.com/trixi-framework/Trixi.jl/blob/main/AUTHORS.md

huiyuxie commented 4 weeks ago

Thanks for the review! Please wait as I am still busy with some other things these days ;)

huiyuxie commented 3 weeks ago

Would it be good to open an issue for this https://github.com/trixi-framework/Trixi.jl/pull/1947#discussion_r1631347563? For later change it back to integer 0

jlchan commented 3 weeks ago

@huiyuxie I am fairly busy this week and at a conference the next week. When do you need this review by?

ranocha commented 3 weeks ago

@huiyuxie I am fairly busy this week and at a conference the next week. When do you need this review by?

I think there is no need for you to review this PR.

huiyuxie commented 3 weeks ago

@jlchan Good luck!

huiyuxie commented 2 weeks ago

Fine, overall it is not a big issue. Moving the whole process forward is much more important to me - this PR looks like holding up other PRs for a long time. I will fix the former check issues directly in this PR.

huiyuxie commented 2 weeks ago

Name added. Ready for final review and approval of merge @ranocha.

trixi-framework / Trixi.jl