Add numerical support of other real types (`compressible_euler`) #1947

huiyuxie · 2024-05-17T21:35:30Z

Continue #1909.

Tasks:

compressible_euler_multicomponent_1d, compressible_euler_multicomponent_2d
compressible_euler_quasi_1d
unit tests for above

github-actions · 2024-05-17T21:35:46Z

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 ❤️ by the Trixi.jl community.

codecov · 2024-05-17T22:13:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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	Coverage Δ
unittests	`96.16% <100.00%> (+0.02%)`	⬆️

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.

huiyuxie · 2024-05-17T23:09:18Z

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

LGTM so far, thanks!

ranocha · 2024-05-19T15:14:03Z

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 #1905 and codecov/feedback#301

huiyuxie · 2024-05-21T01:26:37Z

@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 · 2024-05-21T04:26:49Z

That PR has too many comments so I comment here.

ranocha · 2024-05-21T13:36:30Z

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.

src/equations/compressible_euler_quasi_1d.jl

ranocha

Thanks. This looks good so far. I think it would be good to wait until the other PR is merged to get the infrastructure for unit tests

src/equations/compressible_euler_quasi_1d.jl

ranocha · 2024-06-03T12:21:37Z

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 · 2024-06-04T02:21:58Z

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

huiyuxie · 2024-06-05T23:52:56Z

test/test_type.jl

+            @test eltype(@inferred cons2prim(u, equations)) == RealT
+            @test eltype(@inferred prim2cons(u, equations)) == RealT
+            # TODO: There is a bug in the `entropy` function
+            # @test eltype(@inferred entropy(u, equations)) == RealT


Trixi.jl/src/equations/compressible_euler_quasi_1d.jl

Lines 315 to 321 in 8665300

@inline function entropy(u, equations::CompressibleEulerEquationsQuasi1D)

a_rho, a_rho_v1, a_e, a = u

q = a * entropy(SVector(a_rho, a_rho_v1, a_e) / a,

CompressibleEulerEquations1D(equations.gamma))

return SVector(q[1], q[2], q[3], a)

end

The entropy for CompressibleEulerEquations1D does not return as a vector. Who was doing the code review?Seriously this is an apparent bug :(

src/equations/compressible_euler_multicomponent_1d.jl

ranocha

Thanks. Please find below some comments

src/equations/compressible_euler_multicomponent_2d.jl

src/equations/compressible_euler_multicomponent_1d.jl

test/test_type.jl

src/equations/compressible_euler_multicomponent_2d.jl

…into main-continue

huiyuxie · 2024-06-11T05:04:43Z

Would it be good to open an issue for this #1947 (comment)? For later change it back to integer 0

jlchan · 2024-06-11T13:54:14Z

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

ranocha

Thanks a lot! Could you please add your name to the list of authors?

test/test_type.jl

ranocha · 2024-06-11T17:31:47Z

@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 · 2024-06-12T03:28:47Z

@jlchan Good luck!

Co-authored-by: Hendrik Ranocha <[email protected]>

huiyuxie · 2024-06-14T07:02:43Z

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.

test/test_type.jl

huiyuxie · 2024-06-14T07:53:32Z

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

ranocha

Thanks a lot!

start main continue

dbad996

huiyuxie requested a review from ranocha May 17, 2024 21:35

huiyuxie self-assigned this May 17, 2024

huiyuxie added the enhancement New feature or request label May 17, 2024

huiyuxie added 2 commits May 17, 2024 15:43

rerun CI tests

cc38cde

test CI tests

92b78e8

huiyuxie mentioned this pull request May 18, 2024

Support type stability for Float32 when calling rhs! trixi-gpu/TrixiCUDA.jl#12

Closed

ranocha previously approved these changes May 19, 2024

View reviewed changes

huiyuxie added 2 commits May 27, 2024 22:09

Merge branch 'main' into main-continue

7e068bc

complete the first

0b9589e

huiyuxie dismissed ranocha’s stale review via 0b9589e May 28, 2024 05:24

huiyuxie requested a review from ranocha May 28, 2024 05:25

huiyuxie commented May 28, 2024

View reviewed changes

src/equations/compressible_euler_quasi_1d.jl Show resolved Hide resolved

ranocha reviewed May 28, 2024

View reviewed changes

src/equations/compressible_euler_quasi_1d.jl Show resolved Hide resolved

Merge branch 'main' into main-continue

f80e030

compressible euler quasi 1d

8aa7542

huiyuxie requested a review from ranocha June 5, 2024 23:39

huiyuxie commented Jun 5, 2024

View reviewed changes

compressible euler multicomponent 1d

6f2b008

huiyuxie commented Jun 6, 2024

View reviewed changes

src/equations/compressible_euler_multicomponent_1d.jl Show resolved Hide resolved

compressible euler multicomponent 2d

0cecb0e

complete unit tests

2249e59

ranocha requested changes Jun 7, 2024

View reviewed changes

huiyuxie added 3 commits June 7, 2024 21:41

Merge branch 'main' into main-continue

b3c3b3a

fix comments

b1c167b

Merge branch 'main-continue' of https://github.com/huiyuxie/Trixi.jl …

c0d27a9

…into main-continue

huiyuxie requested a review from ranocha June 10, 2024 03:56

jlchan mentioned this pull request Jun 10, 2024

Fix entropy for quasi-1D compressible Euler #1974

Merged

huiyuxie added 2 commits June 10, 2024 19:02

Merge branch 'main' into main-continue

7f5057e

complete

930815c

huiyuxie requested a review from jlchan June 11, 2024 05:09

ranocha requested changes Jun 11, 2024

View reviewed changes

test/test_type.jl Outdated Show resolved Hide resolved

test/test_type.jl Outdated Show resolved Hide resolved

test/test_type.jl Outdated Show resolved Hide resolved

test/test_type.jl Outdated Show resolved Hide resolved

huiyuxie and others added 3 commits June 11, 2024 20:29

Merge branch 'main' into main-continue

89f9592

apply strict type check

b6bd36e

Co-authored-by: Hendrik Ranocha <[email protected]>

Merge branch 'main' into main-continue

a1acb1e

cover all

c74b5a4

huiyuxie commented Jun 14, 2024

View reviewed changes

test/test_type.jl Show resolved Hide resolved

cover before

307c2ec

huiyuxie requested a review from ranocha June 14, 2024 07:47

add me

71b7a64

ranocha approved these changes Jun 14, 2024

View reviewed changes

ranocha merged commit 7b57dd5 into trixi-framework:main Jun 14, 2024
37 checks passed

huiyuxie deleted the main-continue branch June 21, 2024 07:56

huiyuxie changed the title ~~Add numerical support of other real types (continue)~~ Add numerical support of other real types (compressible_euler) Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add numerical support of other real types (`compressible_euler`) #1947

Add numerical support of other real types (`compressible_euler`) #1947

huiyuxie commented May 17, 2024 •

edited

Loading

github-actions bot commented May 17, 2024

codecov bot commented May 17, 2024 •

edited

Loading

huiyuxie commented May 17, 2024 •

edited

Loading

ranocha left a comment

ranocha commented May 19, 2024

huiyuxie commented May 21, 2024 •

edited

Loading

huiyuxie commented May 21, 2024

ranocha commented May 21, 2024

ranocha left a comment

ranocha commented Jun 3, 2024

huiyuxie commented Jun 4, 2024

huiyuxie Jun 5, 2024 •

edited

Loading

ranocha left a comment

huiyuxie commented Jun 11, 2024

jlchan commented Jun 11, 2024

ranocha left a comment

ranocha commented Jun 11, 2024

huiyuxie commented Jun 12, 2024

huiyuxie commented Jun 14, 2024 •

edited

Loading

huiyuxie commented Jun 14, 2024

ranocha left a comment

	@inline function entropy(u, equations::CompressibleEulerEquationsQuasi1D)
	a_rho, a_rho_v1, a_e, a = u
	q = a * entropy(SVector(a_rho, a_rho_v1, a_e) / a,
	CompressibleEulerEquations1D(equations.gamma))

	return SVector(q[1], q[2], q[3], a)
	end

Add numerical support of other real types (compressible_euler) #1947

Add numerical support of other real types (compressible_euler) #1947

Conversation

huiyuxie commented May 17, 2024 • edited Loading

github-actions bot commented May 17, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

codecov bot commented May 17, 2024 • edited Loading

Codecov Report

huiyuxie commented May 17, 2024 • edited Loading

ranocha left a comment

Choose a reason for hiding this comment

ranocha commented May 19, 2024

huiyuxie commented May 21, 2024 • edited Loading

huiyuxie commented May 21, 2024

ranocha commented May 21, 2024

ranocha left a comment

Choose a reason for hiding this comment

ranocha commented Jun 3, 2024

huiyuxie commented Jun 4, 2024

huiyuxie Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

ranocha left a comment

Choose a reason for hiding this comment

huiyuxie commented Jun 11, 2024

jlchan commented Jun 11, 2024

ranocha left a comment

Choose a reason for hiding this comment

ranocha commented Jun 11, 2024

huiyuxie commented Jun 12, 2024

huiyuxie commented Jun 14, 2024 • edited Loading

huiyuxie commented Jun 14, 2024

ranocha left a comment

Choose a reason for hiding this comment

Add numerical support of other real types (`compressible_euler`) #1947

Add numerical support of other real types (`compressible_euler`) #1947

huiyuxie commented May 17, 2024 •

edited

Loading

codecov bot commented May 17, 2024 •

edited

Loading

huiyuxie commented May 17, 2024 •

edited

Loading

huiyuxie commented May 21, 2024 •

edited

Loading

huiyuxie Jun 5, 2024 •

edited

Loading

huiyuxie commented Jun 14, 2024 •

edited

Loading