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 signed distance function for Zalesak's disk #15033

Merged

Conversation

mschreter
Copy link
Contributor

@mschreter mschreter commented Apr 6, 2023

This PR proposes to add a signed distance function for Zalesak's disk (Zalesak, S. T. "Fully multidimensional flux-corrected transport algorithms for fluids." Journal of computational physics 31.3 (1979): 335-362.), which is a common benchmark in the level set community.

@mschreter mschreter force-pushed the add_signed_distance_funcion_zalesak_disk branch from 596c12a to 7e70130 Compare April 6, 2023 13:14
Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Since you already have the reference ready, would you mind adding an entry to doc/doxygen/references.bib and then in the documentation of the class cite the paper via @cite ...?

/**
* Signed-distance level set function of a Zalesak's disk.
*
* This function is zero on the disk, negative "inside" and positive
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
* This function is zero on the disk, negative "inside" and positive
* This function is zero on the surface of the disk, negative "inside" and positive

Comment on lines 374 to 375
std::numeric_limits<
double>::lowest() /* notch is open in y-direction*/,
Copy link
Member

Choose a reason for hiding this comment

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

Nice job -- very few people know about the difference between std::numeric_limits::lowest() and std::numeric_limit::min() and either incorrectly use the latter, or give up and use -std::numeric_limits::max() :-)

Comment on lines 391 to 398
AssertThrow(
notch_width <= 2 * radius,
ExcMessage(
"The width of the notch must be less than the circle diameter. Abort..."));
AssertThrow(
notch_height <= 2 * radius,
ExcMessage(
"The height of the notch must be less than the circle diameter. Abort..."));
Copy link
Member

Choose a reason for hiding this comment

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

Let's just use regular Assert here and drop the last half-sentence (our error messages typically describe what is wrong -- a state -- rather than what will happen -- an action):

Suggested change
AssertThrow(
notch_width <= 2 * radius,
ExcMessage(
"The width of the notch must be less than the circle diameter. Abort..."));
AssertThrow(
notch_height <= 2 * radius,
ExcMessage(
"The height of the notch must be less than the circle diameter. Abort..."));
Assert(
notch_width <= 2 * radius,
ExcMessage(
"The width of the notch must be less than the circle diameter."));
Assert(
notch_height <= 2 * radius,
ExcMessage(
"The height of the notch must be less than the circle diameter."));

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

A few comments. I'll continue later.

Could we move the picture to https://github.com/dealii/old-website/tree/main/images? We don't need to fill the code repo with pictures.

Comment on lines 302 to 305
Functions::SignedDistance::Sphere<dim> sphere;
BoundingBox<dim> notch;
double radius;
Point<dim> center_sphere;
Copy link
Member

Choose a reason for hiding this comment

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

const?

* Contour surfaces of the signed distance function of a 3D Zalesak's disk
* are illustrated below:
*
* @image html signed_distance_zalesak_disk.png
Copy link
Member

Choose a reason for hiding this comment

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

I guess the right figure is the same for 2D?

Copy link
Member

Choose a reason for hiding this comment

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

What happens in 1D?

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 am not sure, it depends on what Sphere<1> does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zalesak_1d

source/base/function_signed_distance.cc Outdated Show resolved Hide resolved
source/base/function_signed_distance.cc Outdated Show resolved Hide resolved
@bangerth
Copy link
Member

bangerth commented Apr 6, 2023

@mschreter If you send me pictures, I'm happy to add them to the repository.

// case: inside disk
else if (sphere.value(p) <= 0)
{
return std::max(sphere.value(p), -notch.signed_distance(p));
Copy link
Contributor

Choose a reason for hiding this comment

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

I am super-confused about the implementation of the value-function. What you do on this line is a setminus operation with level set functions:
image

(table from http://doi.wiley.com/10.1002/nme.4823).
But then it looks to me like the whole value-function could be just this line. Why do you need the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot, I will have a look at it asap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In our case, the setminus operation results in the correct signed distance function for points lying inside the disk but not necessarily for points outside the disk. This is due to the following reasons: notch actually represents the signed distance function to the bounding box of the notch, i.e., the signed distance function to a closed hyperrectangle. Thus, for points lying outside the disk and in the vicinity of the corner points of the notch (2D: zones 6,7,10 and 11 in the drawing below taken from Mousavi, R. (2014). Level set method for simulating the dynamics of the fluid-fluid interfaces: Application of a Discontinuous Galerkin Method)

we need special treatment to get the correct signed distance function. To this end, I introduced the cases in the remaining lines

// point is within bounds of the notch faces
if (*std::min_element(signed_distances_notch.begin(),
signed_distances_notch.end()) > 0)
{
const double r =
std::sqrt(std::pow(radius, 2) -
std::pow(notch.side_length(dim - 1) - radius, 2));
// inside notch
if ((dim == 2 && notch.point_inside(p)) ||
(dim == 3 && p.distance(center_sphere) <= radius))
{
return std::min(
distance_front_notch_edge,
std::min(distance_bottom_notch_edge,
*std::min_element(signed_distances_notch.begin(),
signed_distances_notch.end())));
}
// case: below top face of notch
else if (dim == 2 ||
(dim == 3 && (std::abs(p[1] - center_sphere[1])) < r))
{
return std::min(distance_bottom_notch_edge,
signed_distances_notch[idx_top]);
}
// case: corner, if the point is outside sphere but within notch
// bounds
else
return std::min(distance_bottom_notch_edge,
distance_front_notch_edge);
}
and
else
{
// consider only points below top face of notch
if (signed_distances_notch[idx_top] > 0)
{
// compute predictor for closest point to sphere.
const auto temp = p + (center_sphere - p) /
(center_sphere.distance(p)) *
sphere.value(p);
// case: notch edge, if the predicted point would be inside the
// notch.
if (notch.signed_distance(temp, 0) <= 0)
return std::min(distance_bottom_notch_edge,
distance_front_notch_edge);
}
// case: sphere
return sphere.value(p);
}
.

I have to admit that the name notch may be a bit misleading here, and maybe that's what led to your confusion.
Since I went through the code lines again and found that they are not very easy to follow unless you are directly involved in the implementation, I will try to come up with a more concise and readable implementation as soon as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid I still don't understand why you need the cases. If you just make the notch, so that it extends outside the disk:
image
I don't think there is any problem with setminus. If you compare the implementation you have now with only using setminus it looks like this:

image

And I really believe the setminus will give the correct sign-distance function, because its gradient will have norm equal to 1, if the two functions you construct it from have gradients with norm equal to 1:
image

Copy link
Contributor Author

@mschreter mschreter Apr 14, 2023

Choose a reason for hiding this comment

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

Convinced. I really could not believe it was that easy, but I am glad it is. Thank you @simonsticko for your efforts. I added your name to the changelog file, I hope that is okay with you as well.

@mschreter
Copy link
Contributor Author

mschreter commented Apr 13, 2023

I've tried to restructure the value() function a bit and added some more explanations. We now distinguish 3 cases in 1D, 4 cases in 2D and 6 cases in 3D. That's the best I could come up with for now. If any of you has further suggestions for improvements, I will be happy to incorporate them. Otherwise I am done from my side.

Thanks to the discussion with @simonsticko the value() function is now a one-liner, which is really cool. Sorry for overcomplicating things at first.

/**
* Nothch described by a rectangle.
*/
const Functions::SignedDistance::Rectangle<dim> notch;
Copy link
Member

Choose a reason for hiding this comment

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

I have pushed a few minor simplifications. In particular, I have converted the notch to a Function. I think this is useful once we want to extract the level-set merging operations to helper functions.

@mschreter Could you rebase? I think the PR should be ready now! @simonsticko Could you take a second look?

@mschreter mschreter force-pushed the add_signed_distance_funcion_zalesak_disk branch from 5d10a22 to 64c6dda Compare April 14, 2023 21:39


/**
* Signed-distance level set function of a Zalesak's disk proposed in
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this "a" should go away, i.e. "Signed-distance level set function of Zalesak's disk proposed in ...".

Otherwise I think this looks great now. 💯

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for your insistence @simonsticko, I think this is a very simple code now. Regarding the added picture, we have #15036, so I guess we should have a decision there soon?

@mschreter mschreter force-pushed the add_signed_distance_funcion_zalesak_disk branch 2 times, most recently from e9a7e46 to 5a8bbfe Compare April 15, 2023 16:17
@peterrum peterrum force-pushed the add_signed_distance_funcion_zalesak_disk branch from 5a8bbfe to d8eaa2e Compare April 15, 2023 18:33
@peterrum
Copy link
Member

Regarding the added picture, we have #15036, so I guess we should have a decision there soon?

For the time being, let's move it to the webpage. I have do that.

@kronbichler kronbichler dismissed bangerth’s stale review April 17, 2023 17:25

The comments by @bangerth have been addressed, so I'm merging now.

@kronbichler kronbichler merged commit 464e8e0 into dealii:master Apr 17, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants