-
Notifications
You must be signed in to change notification settings - Fork 11
CarpetX: fix bug in hermite prolongation #347
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will need some discussion, in particular one wants to be very sure that the coefficients are actually correct (eg were they tested to be "Hermite" like in that they give continuous derivatives?).
56 / T(96), | ||
56 / T(96), | ||
-9 / T(96), | ||
+1 / T(96) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I agree that Hermite of same order needs more points, I suspect that we may need to (same as Carpet) read "order" really as "this many ghost points" to be able to mix operators of different orders using the same number of ghost points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. If we do that, are we forced to only mix low order of Hermite with high order other method?
|
||
extern prolongate_3d_rf2<VC, VC, VC, HERMITE, HERMITE, HERMITE, 1, 1, 1, | ||
extern prolongate_3d_rf2<VC, VC, VC, POLY, POLY, POLY, 1, 1, 1, | ||
FB_NONE> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these are actually the same as Langrangian, does it make sense to completely remove them instead and error out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can do that. If the use want to use 1st order Hermite, it can error out and say please use Poly for 1st order case
assert(y1 == y); | ||
// assert(y1 == y); | ||
const T tol = 100 * std::numeric_limits<T>::epsilon(); | ||
assert(std::abs(y1 - y) <= tol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, but if we carefully chose the test problem such that roundoff is not a problem (see comment in code above) why the (hardcoded!) tol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That's just a quick fix. I can take a look and see if I can make y1==y
work again.
This PR is trying to fix bug in the Hermite type of prolongation.