Update EPA test tolerance.

[hongkai-dai]

This change is 

[hongkai-dai]

+@SeanCurtis-TRI for feature review please.

Reviewable status: 0 of 2 files reviewed, all discussions resolved

[SeanCurtis-TRI]

Quick request (longer change....) But since you chose to change the tests, you kind of invited this on yourself.

Reviewed 2 of 2 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 227 at r1 (raw file):

    }

    const S tol(gjkSolver.distance_tolerance);

BTW Am I interpreting this correctly? You've removed the test-defined tolerance in favor of whatever tolerance the gjk solver has defined. So, this implicitly tests that the solver is producing answers consistent with the tolerance (whatever that may be)?

If that's the case, I have two requests:

1. Document that on this line.
2. I suspect this may not be generally true (although, with your fixes, it may be). It would be nice if the test was repeated several times for varying tolerance values. With the collision code, I found the answer got better with a tightening tolerance, and then got worse. In that case, you'd have to hang on to the tolerance all the way up until you instantiate the GJKSolver_libcccd<S> instance.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 332 at r1 (raw file):

  // By setting gjkSolver.distance_tolerance to the default value (1E-6), the
  // tolerance we get on the closest points are only up to 1E-3
  TestBoxes<double>(1E-3);

BTW If you take my suggestion about testing various tolerances, I'd recommend using the constant<S>::eps*() methods to sample around the precision of float/double according to the number of bits.

[hongkai-dai]

Reviewable status: complete! all files reviewed, all discussions resolved

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 227 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Am I interpreting this correctly? You've removed the test-defined tolerance in favor of whatever tolerance the gjk solver has defined. So, this implicitly tests that the solver is producing answers consistent with the tolerance (whatever that may be)?

If that's the case, I have two requests:

1. Document that on this line.
2. I suspect this may not be generally true (although, with your fixes, it may be). It would be nice if the test was repeated several times for varying tolerance values. With the collision code, I found the answer got better with a tightening tolerance, and then got worse. In that case, you'd have to hang on to the tolerance all the way up until you instantiate the GJKSolver_libcccd<S> instance.

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 332 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW If you take my suggestion about testing various tolerances, I'd recommend using the constant<S>::eps*() methods to sample around the precision of float/double according to the number of bits.

Done.

[SeanCurtis-TRI]

More requests/comments

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hongkai-dai)

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 74 at r2 (raw file):
BTW

There are two tolerance__s__: the solver tolerance and the test_tol.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 78 at r2 (raw file):

  // the objects are separated, the GJK algorithm terminates when the change of
  // distance is below solver_tolerance, which does NOT mean that the separation
  // distance computed by GJK is within solver_tolerance to the true distance,

BTW It would be really nice if you could point to some documentation/comments/code for why this is the case. This is important reasoning that should be captured.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 111 at r2 (raw file):

  // Now check if the distance between p1 and p2 matches with dist.
  EXPECT_NEAR((p_FNa - p_FNb).norm(), std::abs(dist),
              fcl::constants<S>::eps_78());

BTW You've defined two tolerances as inputs to this function, but now you're using a third tolerance for this test.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 113 at r2 (raw file):

              fcl::constants<S>::eps_78());
  // Check if p1 is in sphere 1 and p2 is in sphere 2.
  EXPECT_LE((p_FNa - p_FA).norm(), radius1 + fcl::constants<S>::eps_78());

I'm not sure I care for the <= test. Surely the points should lie on the surface. This test allows the points to be internal.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 114 at r2 (raw file):

  // Check if p1 is in sphere 1 and p2 is in sphere 2.
  EXPECT_LE((p_FNa - p_FA).norm(), radius1 + fcl::constants<S>::eps_78());
  EXPECT_LE((p_FNb - p_FB).squaredNorm(),

BTW Squared norm?

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 117 at r2 (raw file):
The why of this test should be underscored. It's purpose is not immediately obvious (i.e., making sure that the points are the right distance from each other and the sphere centers, but also on the line connecting the sphere centers.

In fact, I'd recommend documenting this up front:

Na and Nb are the witness points on sphere 1 and 2, respectively. They should be 'dist' units apart, located on the surface of their respective spheres, and lie on the line connecting the two sphere centers.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 123 at r2 (raw file):

  // points. There are two reasons
  // 1. The witness points are NOT guaranteed to be unique (consider
  // plane-to-plane contact).

This point seems irrelevant to this test; this test is hard-coded for spheres so "uniqueness" isn't an issue (w.r.t. the underlying geometry). It could be mitigated by adding the prefix: "Generally, the witness points are NOT..."

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 126 at r2 (raw file):

  // 2. Even if there are unique witness points, it is hard to infer the bounds
  // on the computed witness points, based on the tolerance of the solver. This
  // bounds depend on the curvature of the geometries, and I do not have an

"...on the curvature of the geometries near the contact region, and we do not have a general approach to compute that bound."

(Generally replace "I" with "we" across the whole file.)

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 159 at r2 (raw file):

          spheres[i].radius + spheres[j].radius) {
        // Not in collision.
        for (int k = 0; k < static_cast<int>(solver_tolerances.size()); ++k) {

FYI You could also do this:

for (S solver_tolerance : {S(1e-4), S(1e-5), S(1e-6)}) {
}

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 202 at r2 (raw file):
BTW Probably worth drawing attention to the difference in test_tol here, like you did above. E.g.:

For colliding spheres, the solver_tolerance is the bound on the error relative to the true answer.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 208 at r2 (raw file):
FYI Might be worth a tweak of the language to make the failure more clear:

The two spheres failed to collide...

This communicates more clearly the expectation.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 297 at r2 (raw file):

    // When the objects penetrate, the computed distance should be within
    // gjkSOlver.distance_tolerance to the actual distance.

BTW s/O/o

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 337 at r2 (raw file):

  //---------------------------------------------------------------
  //                      Touching contact
  for (int i = 0; i < static_cast<int>(solver_distance_tolerances.size());

FYI Doesn't seem like you're using i beyond indexing into the tolerances. In that case:

for (S solver_distance_tolerance : solver_distance_tolerances) {
...
}

[hongkai-dai]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SeanCurtis-TRI)

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 74 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW

There are two tolerance__s__: the solver tolerance and the test_tol.

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 78 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW It would be really nice if you could point to some documentation/comments/code for why this is the case. This is important reasoning that should be captured.

Hmm, I do not get it. I think this comment itself is clear? I haven't found any proof that if the change of distance is below the tolerance, then the distance itself is within some bound to the true tolerance.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 111 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW You've defined two tolerances as inputs to this function, but now you're using a third tolerance for this test.

That is intentional. The distance between the witness points should match the distance exactly, no matter what solver tolerance we choose.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 113 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'm not sure I care for the <= test. Surely the points should lie on the surface. This test allows the points to be internal.

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 114 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Squared norm?

Done. Good catch

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 117 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The why of this test should be underscored. It's purpose is not immediately obvious (i.e., making sure that the points are the right distance from each other and the sphere centers, but also on the line connecting the sphere centers.

In fact, I'd recommend documenting this up front:

Na and Nb are the witness points on sphere 1 and 2, respectively. They should be 'dist' units apart, located on the surface of their respective spheres, and lie on the line connecting the two sphere centers.

Done. The reason why this test holds is not just for spheres, but for any geometries they should touch after the shifting. Here I use this test because it is trivial to check if two spheres are touching.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 123 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This point seems irrelevant to this test; this test is hard-coded for spheres so "uniqueness" isn't an issue (w.r.t. the underlying geometry). It could be mitigated by adding the prefix: "Generally, the witness points are NOT..."

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 126 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

"...on the curvature of the geometries near the contact region, and we do not have a general approach to compute that bound."

(Generally replace "I" with "we" across the whole file.)

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 159 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI You could also do this:

for (S solver_tolerance : {S(1e-4), S(1e-5), S(1e-6)}) {
}

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 202 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Probably worth drawing attention to the difference in test_tol here, like you did above. E.g.:

For colliding spheres, the solver_tolerance is the bound on the error relative to the true answer.

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 208 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI Might be worth a tweak of the language to make the failure more clear:

The two spheres failed to collide...

This communicates more clearly the expectation.

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 297 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW s/O/o

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 337 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI Doesn't seem like you're using i beyond indexing into the tolerances. In that case:

for (S solver_distance_tolerance : solver_distance_tolerances) {
...
}

Done.

[SeanCurtis-TRI]

Forever picky; I've got a few more refining requests/comments -- but I'd say this is ready for another pair of eyes. How about it +@sherm1?

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hongkai-dai)

a discussion (no related file):
On an unrelated and unblocking note:

Tolerances shouldn't be scalar types (S). It doesn't make sense to have a tolerance that is an autodiff. It would be better to declare tolerances as constants<S>::Real. I haven't brought this up because I'm still getting my brain into the habit of thinking about it. But I wanted to give you the chance to think about it as well.

---

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 74 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done.

You missed the colon and the dropped comma.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 78 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Hmm, I do not get it. I think this comment itself is clear? I haven't found any proof that if the change of distance is below the tolerance, then the distance itself is within some bound to the true tolerance.

You're asserting a fact (solution is not bound by iteration tolerance), but you haven't said why the iteration tolerance isn't a bound on the answer. Again, if this were documented in the gjk implementation and you could just point to it for details, that'd be enough. But some future reader might look at this and think, "Not a bound? That sounds wrong. Why did the programmer think it isn't a bound? I'll change this test."

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 126 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done.

(you missed the "Generally replace 'I' with 'we' across the whole file.)

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 337 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done.

In this case, I would've left the vector in place and just changed the iteration -- after all, you have two different for loops that each iterate through the same tolerance values.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 340 at r3 (raw file):

  //                      Touching contact
  // Test with different solver distance tolerances.
  for (const S solver_distance_tolerance : {S(1E-4), S(1E-5), S(1E-6)}) {

These tolerances never really stress test the doubles.

If, instead, you did constants<S>::eps_12(), constants<S>::eps_34() and constants<S>::eps_78() you'd actually be exploiting double precision when Sis double. As it is, you're using doubles to operate in a single-precision domain.

[sherm1]

Nice! pending a few open comments.

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hongkai-dai)

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 83 at r3 (raw file):

  // If the objects penetrate, the EPA algorithm terminates when the difference
  // between the upper bound of penetration depth and the lower bound
  // is below solver_distance, which means that the EPA computed penetration

Minor: solver_distance -> solver_tolerance ?

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 84 at r3 (raw file):

  // between the upper bound of penetration depth and the lower bound
  // is below solver_distance, which means that the EPA computed penetration
  // depth is within solver_tolerance to the true depth.

Minor: to be clear, are you saying that in contrast to GJK, EPA converging to solver_tolerance bounds the penetration depth error to solver_tolerance? Consider pointing out that difference between the algorithms explicitly -- it is easy to miss here.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 109 at r3 (raw file):

  EXPECT_NEAR(dist, min_distance_expected, test_tol);
  // Now check if the distance between p1 and p2 matches with dist, they should

Minor: there seems to be a random mix of A&B and 1&2 used to distinguish the two points and spheres. Can you stick to one of those here or is there a reason you need both? Would it work to use p_FN1 and p_FN2 rather than a & b?

Currently the comments have 1s and 2s but then the code doesn't match. Whatever notation you choose, the comments and code should agree.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 303 at r3 (raw file):

    // gjkSolver.distance_tolerance to the actual distance.

    EXPECT_NEAR(dist, distance_expected, test_distance_tolerance);

Minor: this is comparing to test_distance_tolerance. The comment above it says it should be within gjkSolver.distance_tolerance. But I see that earlier you set gjkSolver.distance_tolerance=solver_distance_tolerance, not test_distance_tolerance. So it's not clear how the comment relates to the chosen tolerance here. Please clarify.

[sherm1]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hongkai-dai)

a discussion (no related file):

On an unrelated and unblocking note:

(Reviewable says you are actually blocking, @SeanCurtis-TRI.)

Tolerances shouldn't be scalar types (S)

I think you're right about that. But ... could it ever make sense to ask for ∂result/∂tol perhaps as a convergence study of some kind? My thought is that the discrete nature of the algorithms means that the derivative would always be zero for an infinitesimal change of tol so the only way to do this would be to make a (large-ish) finite change to tol+Δtol. If that's right then we should make all the tols doubles (eventually).

---

[SeanCurtis-TRI]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hongkai-dai)

a discussion (no related file):

Previously, sherm1 (Michael Sherman) wrote…

On an unrelated and unblocking note:

(Reviewable says you are actually blocking, @SeanCurtis-TRI.)

Tolerances shouldn't be scalar types (S)

I think you're right about that. But ... could it ever make sense to ask for ∂result/∂tol perhaps as a convergence study of some kind? My thought is that the discrete nature of the algorithms means that the derivative would always be zero for an infinitesimal change of tol so the only way to do this would be to make a (large-ish) finite change to tol+Δtol. If that's right then we should make all the tols doubles (eventually).

I see what you're saying. This is an interesting thought -- when I was visiting these tolerances in the past, we'd discussed that we wouldn't want tolerances of AutoDiff type. For example, constants<AutoDiffXd<double>>::eps() will not return an AutoDiffXd; it'll return a double.

We'll want to ponder this can reconcile the code and the intent.

---

[SeanCurtis-TRI]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hongkai-dai and @sherm1)

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 340 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

These tolerances never really stress test the doubles.

If, instead, you did constants<S>::eps_12(), constants<S>::eps_34() and constants<S>::eps_78() you'd actually be exploiting double precision when Sis double. As it is, you're using doubles to operate in a single-precision domain.

minor: Changing this to minor so it can be changed without me blocking

[hongkai-dai]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI and @sherm1)

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 74 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

You missed the colon and the dropped comma.

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 78 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

You're asserting a fact (solution is not bound by iteration tolerance), but you haven't said why the iteration tolerance isn't a bound on the answer. Again, if this were documented in the gjk implementation and you could just point to it for details, that'd be enough. But some future reader might look at this and think, "Not a bound? That sounds wrong. Why did the programmer think it isn't a bound? I'll change this test."

Done. I added documentation in gjk_libccd-inl.h

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 126 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

(you missed the "Generally replace 'I' with 'we' across the whole file.)

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 337 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

In this case, I would've left the vector in place and just changed the iteration -- after all, you have two different for loops that each iterate through the same tolerance values.

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 83 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: solver_distance -> solver_tolerance ?

Done.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 84 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: to be clear, are you saying that in contrast to GJK, EPA converging to solver_tolerance bounds the penetration depth error to solver_tolerance? Consider pointing out that difference between the algorithms explicitly -- it is easy to miss here.

Done. I add a documentation in the gjk_libccd-inl.h

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 109 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: there seems to be a random mix of A&B and 1&2 used to distinguish the two points and spheres. Can you stick to one of those here or is there a reason you need both? Would it work to use p_FN1 and p_FN2 rather than a & b?

Currently the comments have 1s and 2s but then the code doesn't match. Whatever notation you choose, the comments and code should agree.

Done. I switched to 1 and 2.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 303 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: this is comparing to test_distance_tolerance. The comment above it says it should be within gjkSolver.distance_tolerance. But I see that earlier you set gjkSolver.distance_tolerance=solver_distance_tolerance, not test_distance_tolerance. So it's not clear how the comment relates to the chosen tolerance here. Please clarify.

Done. the documentation above is outdated.

---

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_signed_distance.cpp, line 340 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

minor: Changing this to minor so it can be changed without me blocking

Done.

[sherm1]

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hongkai-dai)

[hongkai-dai]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I see what you're saying. This is an interesting thought -- when I was visiting these tolerances in the past, we'd discussed that we wouldn't want tolerances of AutoDiff type. For example, constants<AutoDiffXd<double>>::eps() will not return an AutoDiffXd; it'll return a double.

We'll want to ponder this can reconcile the code and the intent.

Done.

---

[hongkai-dai]

Dismissed @SeanCurtis-TRI from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved

[sherm1]

Dismissed @SeanCurtis-TRI from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved

[sherm1]

a discussion (no related file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done.

@SeanCurtis-TRI I assumed you meant to unblock this -- apologies if not!

---

[SeanCurtis-TRI]

There is still some residue I'm not completely happy with, but I can come to terms with this merge.

[hongkai-dai]

I can send another PR to resolve the remaining issues. Shall we discuss this tomorrow when you are at the office?

[SeanCurtis-TRI]

Possibly -- at least we can decide if it's even worth doing a follow up. My nitpicks certainly didn't touch the fundamental goodness of this PR. :)