trixi-framework / Trixi.jl

Trixi.jl: Adaptive high-order numerical simulations of conservation laws in Julia
https://trixi-framework.github.io/Trixi.jl
MIT License
505 stars 98 forks source link

Support of other real types (`Float32`, auxiliary) #1929

Closed huiyuxie closed 1 month ago

huiyuxie commented 2 months ago

The first extra part of #1909, mainly addressing src/auxiliary.

TODO: https://github.com/trixi-framework/Trixi.jl/pull/1929#discussion_r1590639060

github-actions[bot] commented 2 months 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

Code quality

Documentation

Testing

Performance

Verification

Created with :heart: by the Trixi.jl community.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 91.13%. Comparing base (c221bca) to head (d7716cb).

Files Patch % Lines
src/auxiliary/math.jl 0.00% 12 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1929 +/- ## ========================================== - Coverage 96.09% 91.13% -4.96% ========================================== Files 453 453 Lines 36481 36489 +8 ========================================== - Hits 35055 33252 -1803 - Misses 1426 3237 +1811 ``` | [Flag](https://app.codecov.io/gh/trixi-framework/Trixi.jl/pull/1929/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/1929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trixi-framework) | `91.13% <0.00%> (-4.96%)` | :arrow_down: | 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

Thanks for the review @ranocha ! I am thinking about how to test these PRs. Generally, I would treat these smaller PRs as preliminary improvements (as the general PR is calling some basic functions here). So basically we could test and merge these smaller PRs first then merge and test the general PR. But what if some PRs are cross-referencing each other (for example, some PRs rely on the improvements from each other)?

I am not sure whether the case I mentioned above is going to happen in these improvements.

ranocha commented 1 month ago

Thanks for the review @ranocha ! I am thinking about how to test these PRs. Generally, I would treat these smaller PRs as preliminary improvements (as the general PR is calling some basic functions here). So basically we could test and merge these smaller PRs first then merge and test the general PR. But what if some PRs are cross-referencing each other (for example, some PRs rely on the improvements from each other)?

I am not sure whether the case I mentioned above is going to happen in these improvements.

In this case, the PRs providing functionality required by other changes should be merged first.

sloede commented 1 month ago

This depends on #1909 (or at least similar issues apply), right? If yes, please re-request my review here once changes in #1909 that are also pertinent for this PR here have been applied here as well 🙂

huiyuxie commented 1 month ago

1909 depends on this PR (and possibly more small PRs like this in the future).

huiyuxie commented 1 month ago

Rerun CI please (if you like). I locally ran CI when these were running here - hitting the limit and thus too many CI failures here.