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 geometric terms for spherical 2D support. #4141

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

zhichen3
Copy link
Contributor

@zhichen3 zhichen3 commented Sep 8, 2024

Summary

This adds the appropriate geometric terms (area and volume) for spherical 2D geometry. We assume dx[0]=dr, and dx[1]=d$\theta$.

Additional background

This intended to deal with issue #3670

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@WeiqunZhang
Copy link
Member

LGTM

@@ -461,7 +513,10 @@ CoordSys::Volume (const Real xlo[AMREX_SPACEDIM],
*(xhi[2]-xlo[2]));
#if (AMREX_SPACEDIM==2)
case RZ:
return static_cast<Real>(0.5*TWOPI)*(xhi[1]-xlo[1])*(xhi[0]*xhi[0]-xlo[0]*xlo[0]);
return static_cast<Real>(0.5*TWOPI)*(xhi[1]-xlo[1])*(xhi[0]+xlo[0])*(xhi[0]-xlo[0]);
Copy link
Member

Choose a reason for hiding this comment

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

this will cause roundoff diffs when merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I change it back?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I changed it back.

@zingale
Copy link
Member

zingale commented Sep 10, 2024

this looks good to me too, but I don't really understand where getEdgeVolCoord is used.

@zhichen3 zhichen3 marked this pull request as ready for review September 10, 2024 13:55
@@ -164,6 +164,7 @@ CoordSys::UpperIndex(const Real* point) const noexcept
IntVect ix;
for (int k = 0; k < AMREX_SPACEDIM; k++)
{
// +1 for UpperIndex?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, I was wondering if UpperIndex should have a +1

Copy link
Member

Choose a reason for hiding this comment

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

it looks like it should, but it also appears that this isn't used anywhere

@@ -164,7 +164,7 @@ CoordSys::UpperIndex(const Real* point) const noexcept
IntVect ix;
for (int k = 0; k < AMREX_SPACEDIM; k++)
{
ix[k] = (int) ((point[k]-offset[k])/dx[k]);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should do +1 to the int, not the real number before the cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the +1 outside int()

@WeiqunZhang
Copy link
Member

this looks good to me too, but I don't really understand where getEdgeVolCoord is used.

getEdgeVolCoord was used in BoxLib's Intepolater. But at one point, we decided that amrex will only support 1d spherical, 2d rz, cartesian coordinates, and inlined the edge vol corrds into the interpolation code for simplicity.

For you 2D, spherical, you might need to modify interpolaters for better accuracy.

@WeiqunZhang
Copy link
Member

You want to make sure CellConservativeLinear does conserve mass in 2d spherical coordinates.

@WeiqunZhang WeiqunZhang merged commit 0286385 into AMReX-Codes:development Sep 12, 2024
59 checks passed
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.

3 participants