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

p-adaptation finalized for multi-material #626

Merged
merged 19 commits into from
Aug 19, 2024
Merged

p-adaptation finalized for multi-material #626

merged 19 commits into from
Aug 19, 2024

Conversation

adityakpandare
Copy link
Member

@adityakpandare adityakpandare commented Aug 13, 2024

In addition to @WeizhaoLi2018's p-adaptive multimaterial changes, this change-set also removes the old least-squares (LS) reconstruction functions that used only the face-neighbors. Why:

  1. It was tested that this choice of neighbor-stencil can be unstable. The better choice is to use node-neighbors, which is expensive due to the large neighborhood, but very stable.
  2. It wasn't being used.
  3. The upkeep of this LS scheme was turning out to be more than initially thought.

Removing the face-neighbor LS means that compflow and transport cannot work with scheme="p0p1" anymore.


This change is Reviewable

Copy link
Collaborator

@WeizhaoLi2018 WeizhaoLi2018 left a comment

Choose a reason for hiding this comment

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

Reviewed 75 of 75 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @adityakpandare)


src/PDE/DGPDE.hpp line 199 at r1 (raw file):

                           tk::Fields& prim,
                           std::size_t nielem,
													 std::vector< std::size_t >& ndofel ) const

Tab format issue


src/PDE/PrefIndicator.cpp line 91 at r1 (raw file):

  auto epsH = std::pow(10, -4 - tolref * 4.0);
  auto epsL = std::pow(10, -6 - tolref * 8.0);
  //auto epsL_p2 = std::pow(10, -7 - tolref * 8.0);

Here the epsL_p2 as well as the "else if" part below are left here since I observed that sometimes just use epsL for both DGP2 and DGP1 might not produce good ndof distributions. You can perform some further testings by manupulating these three thresholds.


src/PDE/PrefIndicator.cpp line 400 at r1 (raw file):

  }

  Ind = dU / U;

Do I remove this indicator based on velocity?


src/PDE/Reconstruction.cpp line 35 at r1 (raw file):

void
lhsLeastSq_P0P1( const inciter::FaceData& fd,

Do we remove all these functions or transfer into other places?


tests/regression/inciter/transport/GaussHump/CMakeLists.txt line 19 at r1 (raw file):

                    LABELS alecg)

add_regression_test(gauss_hump_p0p1 ${INCITER_EXECUTABLE}

Do we remove all these regression testing?

Copy link
Member Author

@adityakpandare adityakpandare left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @WeizhaoLi2018)


src/PDE/DGPDE.hpp line 199 at r1 (raw file):

Previously, WeizhaoLi2018 (Weizhao Li) wrote…

Tab format issue

Done.


src/PDE/PrefIndicator.cpp line 91 at r1 (raw file):

Previously, WeizhaoLi2018 (Weizhao Li) wrote…

Here the epsL_p2 as well as the "else if" part below are left here since I observed that sometimes just use epsL for both DGP2 and DGP1 might not produce good ndof distributions. You can perform some further testings by manupulating these three thresholds.

Thanks for the note. I made a comment in the code to this effect.


src/PDE/PrefIndicator.cpp line 400 at r1 (raw file):

Previously, WeizhaoLi2018 (Weizhao Li) wrote…

Do I remove this indicator based on velocity?

Yes, the mm_padap branch removed velocity-based indicator. I was also curious about that. Do you remember why?


src/PDE/Reconstruction.cpp line 35 at r1 (raw file):

Previously, WeizhaoLi2018 (Weizhao Li) wrote…

Do we remove all these functions or transfer into other places?

Removed all of these. They are never used for multimat. Only for compflow, which can also use the extended stencil version below.


tests/regression/inciter/transport/GaussHump/CMakeLists.txt line 19 at r1 (raw file):

Previously, WeizhaoLi2018 (Weizhao Li) wrote…

Do we remove all these regression testing?

Yes, because we removed the face-neighbor p0p1 least-squares reconstruction, I removed the testing for it. To put it back, we will have to update the baselines with the node-neighbor least-squares. I can try to do that eventually when I get a chance, but for now, this will have to go.

Copy link
Collaborator

@WeizhaoLi2018 WeizhaoLi2018 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 73 of 75 files reviewed, 1 unresolved discussion (waiting on @adityakpandare)


src/PDE/PrefIndicator.cpp line 400 at r1 (raw file):

Previously, adityakpandare (Aditya Pandare) wrote…

Yes, the mm_padap branch removed velocity-based indicator. I was also curious about that. Do you remember why?

If I removed it, it must be based on the results that showing this does not contribute to the final Ind we are using.

@adityakpandare adityakpandare merged commit f052508 into develop Aug 19, 2024
1 of 2 checks passed
@adityakpandare adityakpandare deleted the mm_padap branch August 19, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants