This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 781 - Provide actual unit tests for autodiff-module
Summary: Provide actual unit tests for autodiff-module
Status: ASSIGNED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Unsupported modules (show other bugs)
Version: 3.2
Hardware: All All
: Low Unit Test Required
Assignee: Matt
URL:
Whiteboard:
Keywords: JuniorJob, test-needed
Depends on:
Blocks:
 
Reported: 2014-04-01 16:56 UTC by Matt
Modified: 2019-12-04 13:11 UTC (History)
3 users (show)



Attachments
Patch (diff) (75 bytes, application/octet-stream)
2014-04-01 16:56 UTC, Matt
no flags Details

Description Matt 2014-04-01 16:56:45 UTC
Created attachment 448 [details]
Patch (diff)

There's a minor bug (typo) in "unsupported/test/autodiff.cpp" which prevents the agreement of `Vector2f` and `AutoDiffScalar<Vector2f>` results in function `test_autodiff_vector`.

Namely, this is the function call for the first case:
`foo<Vector2f>(Vector2f(1,2))`

While this is the argument for the second case:
`VectorAD p(AD(1),AD(-1));`
and its subsequent use:
`AD res = foo<VectorAD>(p);`.

This results in `res.value()` (and derivatives, naturally), to deviate from the non-AD result (which has been computed for (1,2) -- but is then compared with (1,-1)).

A proposed minimal patch changes the following definition:
`VectorAD p(AD(1),AD(-1));`
to this:
`VectorAD p(AD(1),AD(2));`.

This results in the test producing results that are in agreement.

// Confirmed with Eigen 3.2.1, may also affect other versions.
Comment 1 Christoph Hertzberg 2014-04-01 17:51:12 UTC
The test did not test anything at all. I modified it to make some actual comparisons (still far from a complete test-suite -- especially the derivations are not checked at all).

https://bitbucket.org/eigen/eigen/commits/8f8775c
https://bitbucket.org/eigen/eigen/commits/2f1d20d

I'm renaming this bug -- if someone wants to provide a more extensive test-suite, please contribute :)
Comment 2 Matt 2014-04-01 21:25:48 UTC
I may (or may not :]) take a look!

I'm wondering, is there any preferred approach in this case?

One can:

- verify against manually/symbolically-derived expressions
// assumption: we're trusting these to be derived correctly ;-)

- verify against against the results obtained from the Numerical Differentiation module
// assumption: we're trusting these to be correct / close-enough (I imagine we'd still use VERIFY_IS_APPROX either way)
// note: introduces coupling between these modules for the test(s) (unsure whether this is a serious issue)

Thoughts?
Comment 3 Christoph Hertzberg 2014-04-02 10:53:32 UTC
(In reply to comment #2)
> I may (or may not :]) take a look!

Great!

> - verify against manually/symbolically-derived expressions
> // assumption: we're trusting these to be derived correctly ;-)

In general, I would avoid hardcoded tests where possible. One alternative here would be to test product-, sum-, chain-, ... rules for random functions -- but I guess that could be a bit over-complicating, and we'd need hardcoded tests for basic functions (sin, cos, exp, ...) anyways.

> - verify against against the results obtained from the Numerical
> Differentiation module

Yes, I think that would be a good idea.

> // assumption: we're trusting these to be correct / close-enough (I imagine
> we'd still use VERIFY_IS_APPROX either way)

Of course, as soon as floating point math is involved, we need IS_APPROX. A question is whether we need to increase the tolerance for this.

> // note: introduces coupling between these modules for the test(s) (unsure
> whether this is a serious issue)

I rather think that is a good thing. If a unit test fails, due to changes in either module it should be easy to detect which module caused it.
Comment 4 Matt 2014-04-05 17:13:58 UTC
OK, here's the temporary / work-in-progress draft version:
https://bitbucket.org/MattPD/eigen-tests/commits/f978023

It's in a separate / private repo at the moment of writing since I don't think it's ready to go yet (I've started with only addressing `test_autodiff_scalar` for starters -- and also temporarily added diagnostic output for debugging & reporting purposes, see below) -- naturally, I gave you and Gael read access.

// And I don't believe I can just commit to Eigen anyway; I presume I can just send a pull request when done; haven't used Bitbucket before, but have used GitHub, so feel free to correct my git-centrism when appropriate, please ;-)

As we expected, we may want to increase tolerance; otherwise, we can experience concessional failures.

For instance:

Initializing random number generator with seed 1396708072
Repeating each test 10 times
test_autodiff_scalar:
Scalar foo(const Scalar&, const Scalar&) [with Scalar = float]
1.46895
Scalar foo(const Scalar&, const Scalar&) [with Scalar = Eigen::AutoDiffScalar<Ei
gen::Matrix<float, 2, 1> >]
1.46895 <> -2.056  1.283

Scalar foo(const Scalar&, const Scalar&) [with Scalar = float]
Scalar foo(const Scalar&, const Scalar&) [with Scalar = float]
Scalar foo(const Scalar&, const Scalar&) [with Scalar = float]
Scalar foo(const Scalar&, const Scalar&) [with Scalar = float]
Scalar foo(const Scalar&, const Scalar&) [with Scalar = float]
grad: -2.054  1.283
Test autodiff failed in DemoAutoDiff.cpp (183)
    test_isApprox(res.derivatives().transpose(), grad)
Stack:
  - autodiff
  - Seed: 1396708072

I also have to ask, just to make sure -- is the current write-a-functor-and-use-inheritance approach (following the test in `NumericalDiff.cpp`) the "right way" to use the NumericalDiff module? I find it somewhat imposing, it has also significantly increased the size (and complexity) of the source code. Is there a hidden Boost.ResultOf-style (or automatic function-to-functor adaptor) functionality that I should be using instead?

// If not, perhaps I could take a stab at implementing one, but can't promise an immediate commit of this one :]

Thoughts?
Comment 5 Christoph Hertzberg 2014-06-15 19:52:18 UTC
Matt, are you still working on this, or shall someone take over? (If so, re-assign to `nobody`)

Regarding your question about NumericalDiff, I added Thomas to the cc-list, maybe he can explain his design intentions.
Comment 6 Matt 2014-06-15 23:44:44 UTC
Christoph, been a bit busy the last couple of weeks, but yes!

I've also run into another issue: there might have been a regression in the commit `2f1d20d` related to activating `test_autodiff_jacobian();` (note that it's been previously commented out). I've encountered build problems with MSVC 2013 (but not with TDM-GCC x64 4.8.1).

I'm currently investigating -- and may have to roll back the activation of `test_autodiff_jacobian` -- but first wanted to confirm (and test on different configurations / platforms, including older MSVC versions). Should have some time around the next weekend.

Incidentally, would you happen to know (or know who knows) the background before the initial removal/deactivation of `test_autodiff_jacobian` -- perhaps it would be helpful to know in this context?
Comment 7 Christoph Hertzberg 2014-06-16 00:12:23 UTC
Thanks -- and there is currently no rush!

I had no issues running that function (built with various gcc's), so I just reactivated it.
If it is of any help, this is the commit that previously deactivated it:
https://bitbucket.org/eigen/eigen/commits/6eea12b
Comment 8 Nobody 2019-12-04 13:11:04 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/781.

Note You need to log in before you can comment on or make changes to this bug.