[Einstein Toolkit] #1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too.
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. --------------------------------+------------------------------------------- Reporter: koppel@… | Owner: Type: defect | Status: new Priority: unset | Milestone: Component: Other | Version: development version Keywords: | --------------------------------+------------------------------------------- When using EinsteinInitialData/TwoPunctures with CCTK_REAL set to single- precision memory-access errors and incorrect results were encountered. The incorrect results could potentially affect double-precision code too.
The memory access errors occur due to double* / CCTK_REAL* confusion.
The incorrect results are due to rounding errors in PunctTaylorExpandAtArbitPosition that push an acos argument outside of [-1,1]. Though this was encountered with single-precision code, perhaps it could occur with DP code too.
The attached patch fixes the type mismatch and clamps the acos argument to [-1,1].
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. ---------------------------------+------------------------------------------ Reporter: koppel@… | Owner: Type: defect | Status: review Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ---------------------------------+------------------------------------------ Changes (by eschnett):
* status: new => review
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. ---------------------------------+------------------------------------------ Reporter: koppel@… | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ---------------------------------+------------------------------------------ Changes (by rhaas):
* status: review => reopened
Comment:
Looks mostly ok to me. Minor issues that should be addressed:
* layout of whitespace in the clamp routine should mimic what TwoPunctures' other code uses * I would rename the routine clamptoone to make it clear to which range it clamps * I would add a comment to the clamp on tan() stating that yes indeed the result of this compuation (ie B) is expected to be in the range of [-1,1] which tan is not normally guaranteed to be
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. ---------------------------------+------------------------------------------ Reporter: koppel@… | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ---------------------------------+------------------------------------------
Comment (by koppel@…):
I've addressed the comments and updated the patch.
clamp -> clamp_pm_one. I've also harmonized the formatting with surrounding code.
I've added comments explaining why B should be in [-1,1].
And, just to be safe I added assertions checking for NaNs to PunctTaylorExpandAtArbitPosition and also to PunctEvalAtArbitPositionFast since there are several functions, including square roots, that might cause trouble.
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. ---------------------------------+------------------------------------------ Reporter: koppel@… | Owner: Type: defect | Status: review Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ---------------------------------+------------------------------------------ Changes (by David Koppelman <koppel@…>):
* status: reopened => review
Comment:
I'm not sure whether I'm the one that should set the review flag.
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. ---------------------------------+------------------------------------------ Reporter: koppel@… | Owner: Type: defect | Status: reviewed_ok Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ---------------------------------+------------------------------------------ Changes (by rhaas):
* status: review => reviewed_ok
Comment:
Yes :-). Review basically means that you think it is now ready for someone else to look at. Looks good to me. If you can: please apply. Otherwise I will apply it for you (and you'll be listed as author in git).
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. ---------------------------------+------------------------------------------ Reporter: koppel@… | Owner: Type: defect | Status: reviewed_ok Priority: unset | Milestone: Component: Other | Version: development version Resolution: | Keywords: ---------------------------------+------------------------------------------
Comment (by David Koppelman <koppel@…>):
AFAIK I don't have check-in privileges on the ET repo, so please check it in for me.
#1894: EinsteinInitialData/TwoPunctures segfaults and miscalcs with SP, maybe DP too. ---------------------------------+------------------------------------------ Reporter: koppel@… | Owner: Type: defect | Status: closed Priority: unset | Milestone: Component: Other | Version: development version Resolution: fixed | Keywords: ---------------------------------+------------------------------------------ Changes (by rhaas):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Fixed in git hash d727a41 of einsteininitial.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit