Sort unstructured boundary conditions #583

andrewwinters5000 · 2021-05-20T09:28:47Z

Improve the performance and type stability of the calc_boundary_flux of the unstructured solver. Addresses part of #542

…d information, printing still works in all dimensinos

…ndary condition types

codecov · 2021-05-20T10:45:55Z

Codecov Report

Merging #583 (28f9e59) into main (99a7e28) will decrease coverage by 0.00%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##             main     #583      +/-   ##
==========================================
- Coverage   93.14%   93.14%   -0.01%     
==========================================
  Files         138      139       +1     
  Lines       14082    14111      +29     
==========================================
+ Hits        13117    13143      +26     
- Misses        965      968       +3

Flag	Coverage Δ
unittests	`93.14% <88.46%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
src/equations/compressible_euler_2d.jl	`94.33% <ø> (-0.01%)`	⬇️
src/solvers/dg_unstructured_quad/dg.jl	`75.00% <ø> (ø)`
...emidiscretization/semidiscretization_hyperbolic.jl	`89.01% <66.66%> (+0.37%)`	⬆️
src/solvers/dg_unstructured_quad/dg_2d.jl	`95.89% <90.47%> (-1.89%)`	⬇️
src/mesh/mesh_io.jl	`95.20% <100.00%> (+0.02%)`	⬆️
...s/dg_unstructured_quad/sort_boundary_conditions.jl	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bdcf0d...28f9e59. Read the comment docs.

ranocha

Thanks for working on this! Please find below some comments. Feel free to ping me here or on Slack to discuss anything further.

src/semidiscretization/semidiscretization_hyperbolic.jl

sloede

LGTM in general, with a minor comment. Once the other comments by @ranocha are resolved, this can be reviewed once more and then merged I think.

src/solvers/dg_unstructured_quad/dg_2d.jl

ranocha

Looks already quite good. Just a minor comment and we can merge it from my side.

A general comment: Please consider using the GitHub feature to apply proposed changes directly. You would have saved both of us quite some time by doing that for #570 (comment) where I already fixed the problematic allocations I fixed in 875ad6c

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

andrewwinters5000 · 2021-05-21T13:12:28Z

Looks already quite good. Just a minor comment and we can merge it from my side.

A general comment: Please consider using the GitHub feature to apply proposed changes directly. You would have saved both of us quite some time by doing that for #570 (comment) where I already fixed the problematic allocations I fixed in 875ad6c

Okay, I will start using this feature. I used to like and combine the changes into a single commit to avoid cluttering the git log. But now that we use squash merge for PRs this is no longer an issue

ranocha · 2021-05-21T13:16:46Z

Okay, I will start using this feature. I used to like and combine the changes into a single commit to avoid cluttering the git log. But now that we use squash merge for PRs this is no longer an issue

You can go to the "Files changed" section at the top and select something like "Add suggestion to batch" for all of them. Then, you can commit them in one batch/commit.

andrewwinters5000 · 2021-05-21T13:18:06Z

You can go to the "Files changed" section at the top and select something like "Add suggestion to batch" for all of them. Then, you can commit them in one batch/commit.

Good to know. I will make sure I use this in the future

src/semidiscretization/semidiscretization_hyperbolic.jl

sloede · 2021-05-21T14:50:13Z

src/solvers/dg_unstructured_quad/dg_2d.jl

+  # recursively call this method with the unprocessed boundary types
+  calc_boundary_flux_by_type!(cache, t, remaining_boundary_conditions,
+                              remaining_boundary_condition_indices, mesh, equations, dg)


TBH, I don't really like these fancy lisp-style iterations unless explicitly required for performance reasons. I am not sure if this was benchmarked or not, but it makes the code harder to read and understand (IMHO, almost always does). No objections against using it here, but if we can get away without these constructs except for some extremely critical cases, I'd appreciate it.

efaulhaber · 2021-05-21T21:54:48Z

The changes in this PR seem to add some more complexity. How does the performance compare to the stuff benchmarked by @ranocha in #573?

sloede · 2021-05-21T22:25:03Z

It actually improves the performance, according to some benchmarks shown to me by @andrewwinters5000

ranocha · 2021-05-22T05:58:01Z

The high-level overview: The first version implemented by @andrewwinters5000 payed the price of dynamic (runtime) dispatch (type instability) for every node on every boundary when at least two different BCs were used. My PR reduced that to runtime dispatch once per boundary. This PR fixed the underlying type instability completely, removing the cost of dynamic dispatch.

sloede · 2021-05-22T06:09:35Z

I'm still surprised though that you can see the difference between the second and third versions. Intuitively, I would've thought this should be negligible.

ranocha · 2021-05-22T07:45:19Z

I'm still surprised though that you can see the difference between the second and third versions. Intuitively, I would've thought this should be negligible.

It had less impact, that's true. A bigger performance improvement came from

A general comment: Please consider using the GitHub feature to apply proposed changes directly. You would have saved both of us quite some time by doing that for #570 (comment) where I already fixed the problematic allocations I fixed in 875ad6c

Nevertheless, I think it's good to avoid type instabilities and train people to get used to some of these design choices.

andrewwinters5000 added 3 commits May 20, 2021 10:51

can now digest the BC dictioary into a tuple of tuples with the sorte…

3ba0a89

…d information, printing still works in all dimensinos

new loop structure in calc_boundary_flux for the sorted tuples of bou…

e8851be

…ndary condition types

fix that one can now restart a periodic UnstructuredQuadMesh simulation

67fbf8c

andrewwinters5000 added 3 commits May 20, 2021 14:14

now use a NamedTuple and unpacking for more clarity

e6cf8e2

fix iterator bug in the calc_boundary_flux

c0e17e2

improve variable names in new digest_boundary_conditions function

4e27d8c

andrewwinters5000 marked this pull request as ready for review May 20, 2021 14:00

andrewwinters5000 requested review from ranocha and sloede May 20, 2021 14:00

ranocha mentioned this pull request May 20, 2021

Consolidate mesh infrastructure after adding structured and unstructured ones #542

Closed

23 tasks

ranocha requested changes May 20, 2021

View reviewed changes

src/semidiscretization/semidiscretization_hyperbolic.jl Outdated Show resolved Hide resolved

src/semidiscretization/semidiscretization_hyperbolic.jl Outdated Show resolved Hide resolved

sloede requested changes May 20, 2021

View reviewed changes

src/solvers/dg_unstructured_quad/dg_2d.jl Outdated Show resolved Hide resolved

andrewwinters5000 and others added 7 commits May 21, 2021 09:07

small changes requested in PR comments

c779dc3

working constructor for the new sorted boundary information struct

65e757b

name change of struct

7bb30f3

Merge branch 'main' into unstruct_bc_performance

b1d0d2c

working version of lispy tuple iteration over the new boundary container

df46504

remove unneeded calc_boundary_flux method

6abecb2

fix allocations in wall bc

875ad6c

andrewwinters5000 requested review from sloede and ranocha May 21, 2021 10:55

ranocha reviewed May 21, 2021

View reviewed changes

src/solvers/dg_unstructured_quad/dg_2d.jl Outdated Show resolved Hide resolved

ranocha reviewed May 21, 2021

View reviewed changes

Update src/solvers/dg_unstructured_quad/dg_2d.jl

28f9e59

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

ranocha enabled auto-merge (squash) May 21, 2021 13:17

ranocha approved these changes May 21, 2021

View reviewed changes

sloede reviewed May 21, 2021

View reviewed changes

sloede approved these changes May 21, 2021

View reviewed changes

ranocha disabled auto-merge May 21, 2021 15:48

ranocha merged commit 84674be into trixi-framework:main May 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort unstructured boundary conditions #583

Sort unstructured boundary conditions #583

andrewwinters5000 commented May 20, 2021

codecov bot commented May 20, 2021 •

edited

Loading

ranocha left a comment

sloede left a comment

ranocha left a comment

andrewwinters5000 commented May 21, 2021

ranocha commented May 21, 2021

andrewwinters5000 commented May 21, 2021

sloede May 21, 2021

efaulhaber commented May 21, 2021

sloede commented May 21, 2021

ranocha commented May 22, 2021

sloede commented May 22, 2021

ranocha commented May 22, 2021

Sort unstructured boundary conditions #583

Sort unstructured boundary conditions #583

Conversation

andrewwinters5000 commented May 20, 2021

codecov bot commented May 20, 2021 • edited Loading

Codecov Report

ranocha left a comment

Choose a reason for hiding this comment

sloede left a comment

Choose a reason for hiding this comment

ranocha left a comment

Choose a reason for hiding this comment

andrewwinters5000 commented May 21, 2021

ranocha commented May 21, 2021

andrewwinters5000 commented May 21, 2021

sloede May 21, 2021

Choose a reason for hiding this comment

efaulhaber commented May 21, 2021

sloede commented May 21, 2021

ranocha commented May 22, 2021

sloede commented May 22, 2021

ranocha commented May 22, 2021

codecov bot commented May 20, 2021 •

edited

Loading