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

Feature: Simple clamp AMR indicator. #1354

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

Conversation

jmark
Copy link
Contributor

@jmark jmark commented Feb 9, 2023

Refines an element when the element average of variable lies within a given interval [a,b].
Simple, stupid, nothing fancy.

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks! Please add tests using the new functionality.

src/solvers/dgsem_tree/indicators.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_2d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_3d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
@jmark jmark marked this pull request as draft February 13, 2023 09:24
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Attention: Patch coverage is 6.49351% with 72 lines in your changes missing coverage. Please review.

Project coverage is 96.16%. Comparing base (0c2c367) to head (5f58a09).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
src/solvers/dgsem_tree/indicators_1d.jl 0.00% 20 Missing ⚠️
src/solvers/dgsem_tree/indicators_2d.jl 0.00% 20 Missing ⚠️
src/solvers/dgsem_tree/indicators_3d.jl 0.00% 20 Missing ⚠️
src/solvers/dgsem_tree/indicators.jl 29.41% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1354      +/-   ##
==========================================
- Coverage   96.34%   96.16%   -0.18%     
==========================================
  Files         470      470              
  Lines       37512    37589      +77     
==========================================
+ Hits        36140    36145       +5     
- Misses       1372     1444      +72     
Flag Coverage Δ
unittests 96.16% <6.49%> (-0.18%) ⬇️

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.

@jmark jmark marked this pull request as ready for review May 10, 2024 11:24
Copy link
Contributor

@DanielDoehring DanielDoehring left a comment

Choose a reason for hiding this comment

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

Maybe consider adding small explanatory comments?

src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_2d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_3d.jl Outdated Show resolved Hide resolved
@DanielDoehring
Copy link
Contributor

Following Hendrik, I think this is ready to be merged once coveralls passes, i.e., once every dimensional case has been tested.

@jmark
Copy link
Contributor Author

jmark commented Sep 4, 2024

Following Hendrik, I think this is ready to be merged once coveralls passes, i.e., once every dimensional case has been tested.

This has to wait till #2055 is merged since with this equations I use this AMR indicator.

@DanielDoehring
Copy link
Contributor

This has to wait till #2055 is merged since with this equations I use this AMR indicator.

Is not the opposite the case - you need this for #2055 ? 🤔

Copy link
Contributor

@DanielDoehring DanielDoehring left a comment

Choose a reason for hiding this comment

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

We should stick to english - or replace type with something like indicator_clamp (I think this convention is used at different places)

src/solvers/dgsem_tree/indicators_2d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_3d.jl Outdated Show resolved Hide resolved
src/solvers/dgsem_tree/indicators_1d.jl Outdated Show resolved Hide resolved
@DanielDoehring DanielDoehring added the enhancement New feature or request label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants