Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cubed sphere test cases #199

Open
wants to merge 95 commits into
base: main
Choose a base branch
from
Open

Add cubed sphere test cases #199

wants to merge 95 commits into from

Conversation

benegee
Copy link
Collaborator

@benegee benegee commented Jul 24, 2024

New test cases:

  • baroclinic instability
  • poor man's tracer advection with AMR (as shown at MOW24)

Requires: trixi-framework/Trixi.jl#1918

To test this in the meantime:

  • Build and install t8code
    • has to be main
  • Build and install libtrixi
    • needs this branch: bg/baroclinic-instability
    • e.g.
      cmake -DCMAKE_INSTALL_PREFIX=~/install/libtrixi-baro \
            -DCMAKE_BUILD_TYPE=DEBUG \
            -DT8CODE_ROOT=~/install/t8code \
            .. 
      make
      make install
  • Build julia project
    • cd libtrixi-julia-baro
    • e.g. ./libtrixi-baro/bin/libtrixi-init-julia --t8code-library ../t8code/lib/libt8.so ../libtrixi-baro
    • JULIA_DEPOT_PATH=./julia-depot julia --project=.
    • Package manager ]
    • pkg> dev --local Trixi
    • leave Julia, cd dev/Trixi, git switch feature-t8code-cubed-sphere-setup, cd -
    • JULIA_DEPOT_PATH=./julia-depot julia --project=.
    • Package manager ]
    • update
    • maybe downgrade XML2_jll: add [email protected]
    • precompile
  • Run test case
    cd `libtrixi-baro
    LIBTRIXI_DEBUG=all ./bin/trixi_controller_baroclinic_c ../libtrixi-julia-baro  \
        ./share/libtrixi/LibTrixi.jl/examples/libelixir_t8code3d_euler_baroclinic_instability.jl

benegee and others added 30 commits February 22, 2024 17:49
add parameter index and only get averaged of the variable at position index
proof of concept for creating data vectors in libelixirs which can be
filled later by external applications
required in this PR, subject to change
demonstrates source terms via database
@benegee benegee changed the title Add t8code baroclinic instability test case Add cubed sphere test cases Nov 20, 2024
@benegee benegee mentioned this pull request Dec 2, 2024
@benegee benegee marked this pull request as ready for review December 20, 2024 07:46
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.71%. Comparing base (8e9d576) to head (63ece7a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #199       +/-   ##
===========================================
+ Coverage   82.43%   95.71%   +13.28%     
===========================================
  Files          21       23        +2     
  Lines         905     1096      +191     
  Branches       52       63       +11     
===========================================
+ Hits          746     1049      +303     
+ Misses        155       43      -112     
  Partials        4        4               
Flag Coverage Δ
unittests 95.71% <100.00%> (+13.28%) ⬆️

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.

@benegee
Copy link
Collaborator Author

benegee commented Dec 20, 2024

Finally! 🎉

@jmark's cubed sphere fix was released with t8code v3.0.1. which made it into t8code_jll.jll v3.0.1, which made it into T8code v0.7.4, which made it into Trixi 0.9.12, and is finally here!

@benegee benegee requested a review from sloede December 20, 2024 09:17
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool example case! Just a few minor notes...

@@ -198,6 +196,7 @@ jobs:
cd build
cmake .. -DCMAKE_INSTALL_PREFIX=../install \
-DCMAKE_BUILD_TYPE=Debug \
-DT8CODE_ROOT=$PWD/../t8code-local/prefix \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why delete this file?

Comment on lines +262 to +267
# provide some data because calc_sources! will already be called during initialization
zero_data = zeros(Float64, nelements*nnodes)
registry[1] = zero_data
registry[2] = zero_data
registry[3] = zero_data
registry[4] = zero_data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation does not seem correct. All entries of registry are assigned the same vector zero_data, i.e., they point to a single data structure.

I see that they will be overwritten later again. However, my feeling is that it would be better to make sure that this is not accidentally considered a valid implementation:

Suggested change
# provide some data because calc_sources! will already be called during initialization
zero_data = zeros(Float64, nelements*nnodes)
registry[1] = zero_data
registry[2] = zero_data
registry[3] = zero_data
registry[4] = zero_data
# provide some data because calc_sources! will already be called during initialization
# Note: the data pointers in the registry will be overwritten before the first real use
zero_data = zeros(Float64, nelements*nnodes)
registry[1] = zero_data
registry[2] = zero_data
registry[3] = zero_data
registry[4] = zero_data

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, I am starting to wonder if storage for source terms should maybe live on the Trixi.jl side instead of the C side. This goes against previous statements I made, and I wouldn't change it now before we have gathered more experience with it. However, it seems to me that it is overly convoluted this way, especially since it's effectively Trixi.jl that needs and controls the memory, not the C side

mesh = Trixi.T8codeMeshCubedSphere(lat_lon_levels, layers, 6.371229e6, 30000.0,
polydeg = 5, initial_refinement_level = 0)

# create the data registry and three vectors for the source terms
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# create the data registry and three vectors for the source terms
# create the data registry and four vectors for the source terms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants