Implement driver interface for internally supported AD libraries. #5499

jppelteret · 2017-11-18T11:47:11Z

This commit adds classes that act as an interface to the drivers of internally supported automatic
differentiation libraries (Adol-C and Sacado).

In a quest to simplify the road to full integration of AD into our library, I have stripped the core of the functions required to interact this these libraries from the (to be submitted) helper functions. As a result, this is, effectively, the last major commit that requires knowledge of the inner mechanics of these libraries. These classes supplement and leverage the existing Marking and ExtractData classes where possible. Although there's no context for the use of these drivers yet, one may refer to the "basic" Adol-C and Sacado test cases that I added previously to see how they're used.

As an added bonus, with this addition I think that I have completely (or nearly completely) abstracted and safely defaulted the AD-related functions and classes. So it should be possible to move from the current situation where the selection of a type_code that references a library that deal.II is not built against results in a compiler error, to one that presents only a run-time error of sorts :-)

davydden · 2017-11-18T12:58:55Z

doc/doxygen/headers/automatic_and_symbolic_differentiation.h

@@ -59,6 +59,9 @@
 * A summary of the files that implement the interface to the supported auto-differentiable
 * numbers is as follows:
 *
+ * - ad_drivers.h: Provides classes that act as drivers to the interface of internally supported 
+ *   automatic differentiation libraries. These are used internally as a intermediary to the


as an intermediary ?

davydden · 2017-11-18T13:04:48Z

include/deal.II/differentiation/ad/ad_drivers.h

+              Vector<ScalarType>            &values);
+
+      /**
+       * Computes the values of the vector field.


values -> gradient?

davydden · 2017-11-18T13:06:53Z

include/deal.II/differentiation/ad/ad_drivers.h

+              Vector<ScalarType>              &values);
+
+      /**
+       * Computes the values of the vector field.


values -> gradient ?

davydden · 2017-11-18T13:09:20Z

include/deal.II/differentiation/ad/ad_drivers.h

+        Assert(independent_variables.size() == n_independent_variables,
+               ExcDimensionMismatch(independent_variables.size(),n_independent_variables));
+
+        double *f = new double();


why not double values = 0.; and then give address to it for adolc? Then you don't need cleanup nor temporary variable. Although here you assume that double is castable to scalar_type. What if scalar_type is std::complex<double>?

Good idea. My implementation was aligned with what's suggested in their documentation, but you're right that this can be optimised in a few places. I've done so here and where possible below.

BTW. scalar_type is always double for this class (see typedef above). I've changed the remaining instances of double to scalar_type though to be consistent. Thanks for pointing this out!

davydden · 2017-11-18T13:11:22Z

include/deal.II/differentiation/ad/ad_drivers.h

+               ExcMessage("The length of the gradient vector must be equal "
+                          "to the number of independent variables."));
+
+        scalar_type *g = new scalar_type[n_independent_variables]; // Use smart pointers or std_cxx11::array?


std::vector< scalar_type> g(n_independent_variables)? Then give g.begin() or something to adolc?

p.s. std::array need compile-time size, which is not the case here.

Yes, the note was an old todo. std::array definitely wouldn't work here.

davydden · 2017-11-18T13:12:28Z

include/deal.II/differentiation/ad/ad_drivers.h

+               ExcMessage("The (symmetric) hessian column length must be equal "
+                          "to the number of independent variables."));
+
+        scalar_type **H = new scalar_type*[n_independent_variables]; // Use smart pointers or std_cxx11::array?


why not use hessian directly? alignment is exactly the same.

Two reasons:

The last parameter to ::hessian must be a double**. There will be no way to safely extract an equivalent data type from FullMatrix.

scalar_type **H is not actually a full matrix, but only the upper triangle of the matrix. Adol-C takes advantage of the symmetry of the 2nd derivative operation.

(1) same as above, have you tried assigning addresses of each rows of FullMatrix? (2) that's not an issue, you just need to copy the part from upper triangle before returning

more precisely, give the address of the first element in each row of FullMatrix

davydden · 2017-11-18T13:13:11Z

include/deal.II/differentiation/ad/ad_drivers.h

+               ExcMessage("The length of the dependent function vector must "
+                          " be equal to the number of dependent variables."));
+
+        scalar_type *f = new scalar_type[n_dependent_variables]; // Use smart pointers or std_cxx11::array?


davydden · 2017-11-18T13:13:18Z

include/deal.II/differentiation/ad/ad_drivers.h

+               ExcMessage("The hessian column length must be equal "
+                          "to the number of independent variables."));
+
+        scalar_type **J = new scalar_type*[n_dependent_variables]; // Use smart pointers or std_cxx11::array?


The last parameter to ::jacobian must be a double**. I couldn't find a way to safely extract an equivalent data type from FullMatrix (i.e. everything I tried broke my tests).

jppelteret · 2017-11-18T18:27:15Z

/run-tests

davydden · 2017-11-18T18:30:46Z

include/deal.II/differentiation/ad/ad_drivers.h

+
+        scalar_type **J = new scalar_type*[n_dependent_variables];
+        for (unsigned int i=0; i<n_dependent_variables; ++i)
+          J[i] = new scalar_type[n_independent_variables];


hm, but can you not just use here a pointer to each row in jacobian (instead of allocating memory)?

i mean J[i] = &jacobian(i,0) should work. FullMatrix is row-major so this should be equivalent to what you do here.

Thanks, the more explicit explanation of what you were inferring was helpful.

davydden · 2017-11-19T08:16:14Z

include/deal.II/differentiation/ad/ad_drivers.h

+#ifdef DEAL_II_WITH_ADOLC
+            ::tapestats(active_tape_index, counts.data());
+#else
+            AssertThrow(false, ExcNotImplemented());


here and below maybe introduce a custom exception with the message along the lines "This function is only available if deal.II is compiled with ADOL-C".

As per your suggestion, I've added a custom error message.

davydden · 2017-11-19T08:17:15Z

include/deal.II/differentiation/ad/ad_drivers.h

+        Assert(independent_variables.size() == n_independent_variables,
+               ExcDimensionMismatch(independent_variables.size(),n_independent_variables));
+        Assert(gradient.size() == n_independent_variables,
+               ExcMessage("The length of the gradient vector must be equal "


i would just do ExcDimensionMismatch as it will give you the actual numbers as opposed to just a message.

Same below in numerous places.

davydden · 2017-11-19T08:23:26Z

failing test seems to be related

all-headers/differentiation/ad/ad_drivers.h.debug (Failed)

jppelteret

Thanks for your suggestions, @davydden. I've added another couple of functions that I've pointed out to you...

jppelteret · 2017-11-19T15:39:32Z

include/deal.II/differentiation/ad/ad_drivers.h

+       *          a different number of fields from which a linearization must be computed.
+       */
+      static void
+      initialize (const unsigned int &n_independent_variables);


New function

same question here about how these static member functions are supposed to work: an initialize() function must initialize something, but because the function is static it can't be an object of the surrounding type, and because the argument is const, that's not it either.

Also: no reference necessary. Same below.

I suppose I could rename this to initialize_global_environment or something like that. Would this be better?

Yes, I think that would make the intent clearer!

jppelteret · 2017-11-19T15:40:18Z

include/deal.II/differentiation/ad/ad_drivers.h

+       */
+      template<typename Stream>
+      static void
+      print_tape_stats(Stream             &stream,


New function

Would there ever be a reason to write to anything other than a std::ostream?

Also, @drwells has tried to work on naming conventions for template arguments, and they typically end in Type -- so maybe StreamType?

jppelteret · 2017-11-19T15:41:10Z

include/deal.II/differentiation/ad/ad_drivers.h

+      is_adolc_number<ADNumberType>::value &&
+      is_tapeless_ad_number<ADNumberType>::value
+      >::type
+      configure_tapeless_mode (const unsigned int n_directional_derivatives)


New function

davydden

LGTM, but let's wait for someone else to review

davydden · 2017-11-19T15:59:12Z

include/deal.II/differentiation/ad/ad_drivers.h

+        Assert(counts.size() >= 18, ExcInternalError());
+        stream
+            << "Tape index: " << tape_index << "\n"
+            << "Number of independent variables: " << counts[0] << "\n"


std::endl ?

std::endl flushes the write buffer, so its a bit wasteful to do this 18 times in succession. I do a final std::flush at the end.

davydden · 2017-11-19T16:01:03Z

include/deal.II/differentiation/ad/ad_drivers.h

+
+      }
+
+#ifdef DEAL_II_WITH_TRILINOS


DEAL_II_WITH_ADOLC?

No, this is enabled if is_sacado_rad_number (so definitely Trilinos)

ah, we don't have a separate DEAL_II_WITH_SACADO, meaning that Sacado is required dependency?

Yes, its been this way for a long time. Sometime in the future I'd like to modularise our Trilinos configuration.

Thanks to #6906, I can now add this finer-grained check :-)

jppelteret · 2017-11-19T16:08:37Z

Thanks for the feedback @davydden!

davydden · 2017-11-19T20:52:58Z

@jppelteret tester is still not happy

===============================   OUTPUT BEGIN  ===============================
all-headers/differentiation/ad/ad_drivers.h.debug: BUILD failed. Output:
Generating differentiation-ad-ad_drivers.h.debug_interrupt_guard.cc
Scanning dependencies of target differentiation-ad-ad_drivers.h.debug
Building CXX object CMakeFiles/differentiation-ad-ad_drivers.h.debug.dir/test_header.cc.o
In file included from /home/bob/source/tests/all-headers/test_header.cc:17:
/home/bob/source/include/deal.II/differentiation/ad/ad_drivers.h:495:9: error: use of undeclared identifier 'trace_on'
        trace_on(tape_index,keep);
        ^
/home/bob/source/include/deal.II/differentiation/ad/ad_drivers.h:523:42: error: unknown type name 'STAT_SIZE'
        std::vector<std::size_t> counts (STAT_SIZE);
                                         ^
/home/bob/source/include/deal.II/differentiation/ad/ad_drivers.h:523:41: warning: parentheses were disambiguated as a function declaration [-Wvexing-parse]
        std::vector<std::size_t> counts (STAT_SIZE);
                                        ^~~~~~~~~~~
/home/bob/source/include/deal.II/differentiation/ad/ad_drivers.h:523:42: note: add a pair of parentheses to declare a variable
        std::vector<std::size_t> counts (STAT_SIZE);
                                         ^
                                         (         )
/home/bob/source/include/deal.II/differentiation/ad/ad_drivers.h:524:11: error: no member named 'tapestats' in the global namespace
        ::tapestats(tape_index, counts.data());
        ~~^
/home/bob/source/include/deal.II/differentiation/ad/ad_drivers.h:915:9: error: no template named 'is_adolc_number'; did you mean 'is_ad_number'?
      !(is_adolc_number<ADNumberType>::value &&
        ^~~~~~~~~~~~~~~
        is_ad_number
/home/bob/source/include/deal.II/differentiation/ad/ad_number_traits.h:442:12: note: 'is_ad_number' declared here
    struct is_ad_number
           ^
1 warning and 4 errors generated.
make[3]: *** [CMakeFiles/differentiation-ad-ad_drivers.h.debug.dir/test_header.cc.o] Error 1
make[2]: *** [CMakeFiles/differentiation-ad-ad_drivers.h.debug.dir/all] Error 2
make[1]: *** [CMakeFiles/differentiation-ad-ad_drivers.h.debug.build.dir/rule] Error 2
make: *** [differentiation-ad-ad_drivers.h.debug.build] Error 2


all-headers/differentiation/ad/ad_drivers.h.debug: ******    BUILD failed    *******

===============================    OUTPUT END   ===============================

The following tests FAILED:
	245 - all-headers/differentiation/ad/ad_drivers.h.debug (Failed)
Errors while running CTest
test FAILED

jppelteret · 2017-11-20T06:03:41Z

Erg. Ok, I understand whats going on here. I'll try to fix this during the week.

bangerth

Here are a few comments. I think that will also inform what's happening further down below. Let me know when you've worked through these comments and want me to take another look.

bangerth · 2017-11-24T02:17:04Z

include/deal.II/differentiation/ad/ad_drivers.h

+    DeclException2 (ExcSupportedDerivativeLevels,
+                    std::size_t, std::size_t,
+                    << "The number of derivative levels that this auto-differentiable number supports is "
+                    << arg1 << ", but it is required that it supports at least " << arg2 << " levels.");


I find this misunderstandable because of the passive voice in the second half of the sentence. What is "it" in "it is required"? Who requires it? A type? A code location?

Also, is it the a-d number or type that doesn't satisfy the requirement?

bangerth · 2017-11-24T02:18:45Z

include/deal.II/differentiation/ad/ad_drivers.h

+       */
+      template<typename Stream>
+      static void
+      print_tape_stats(Stream             &stream,


Would there ever be a reason to write to anything other than a std::ostream?

Also, @drwells has tried to work on naming conventions for template arguments, and they typically end in Type -- so maybe StreamType?

bangerth · 2017-11-24T02:18:56Z

include/deal.II/differentiation/ad/ad_drivers.h

+      template<typename Stream>
+      static void
+      print_tape_stats(Stream             &stream,
+                       const unsigned int &tape_index);


no need for the reference

bangerth · 2017-11-24T02:19:25Z

include/deal.II/differentiation/ad/ad_drivers.h

+       */
+      static void
+      enable_taping(const unsigned int &tape_index,
+                    const bool         &keep_independent_values);


no need for the references for these arguments. Same for the next few functions.

bangerth · 2017-11-27T04:43:33Z

include/deal.II/differentiation/ad/ad_drivers.h

+       * @return The scalar values of the function.
+       */
+      static ScalarType
+      value (const unsigned int            &active_tape_index,


no need for reference here either. check places below as well.

bangerth · 2017-11-27T04:54:19Z

include/deal.II/differentiation/ad/ad_drivers.h

+                Vector<ScalarType>            &gradient);
+
+      /**
+       * Computes the hessian of the scalar field with respect to all independent


bangerth · 2017-11-27T04:55:16Z

include/deal.II/differentiation/ad/ad_drivers.h

+       *            function is recorded.
+       * @param[in] independent_variables The scalar values of the independent
+       *            variables whose sensitivities were tracked.
+       * @param[out] gradient The scalar values of the dependent function's gradients.


Does the gradient variable have to have the correct size already, or is it resized?

Same below for the Hessian matrix.

bangerth · 2017-11-27T04:59:44Z

include/deal.II/differentiation/ad/ad_drivers.h

+     * @author Jean-Paul Pelteret, 2017
+     */
+    template<typename ADNumberType, typename ScalarType, typename T = void>
+    struct TapedDrivers


I may just not have read far enough down to see the specializations. But all of the member functions of this class are static, and none actually take any non-trivial arguments. What are they supposed to work on?

I presume that you are in essence defining an interface here, so in specializations of this class, the argument lists are going to consist of elementary data types as well (as opposed to including arguments of type ADNumberType). Are functions in specializations not static then, or how does this work? (And if there is an answer, could it be put into this class's documentation?)

I fully understand what you find wrong with this, but for now how I can best motivate this interface is to reiterate that this the minimal ("sane") interface to a set of background operations that I have found to consistently support both ADOL-C and Sacado. The user should never end up calling any of these functions directly, and they will be used in the appropriate manner by the high-level helper classes.

For Sacado types, we set up, manipulate and extract data directly from each number. The one exception is here - a call to a global function.

For ADOL-C, this is not the case. They have some global variables lying around that are directly manipulated by some global functions. See, for example, here (taped mode) and and here (tapeless mode). In my mind there are no "elementary" objects to manipulate. They're managed by ADOL-C itself. (You will see later that I have put in place many things to manage their management structures :-)

So in summary, I need some specialisations for some functions based on an AD number type. In some cases these functions don't actually operate on the numbers themselves (rather, they configure the environment) so it would be disingenuous to pass in an AD number as a parameter when its not actually being manipulated. Specialising functions through the templated struct + static function idiom seemed the best compromise for all of the use cases for the "driver" functions.

The hope is that being able to write code like

template<int dim, enum AD::NumberTypes ADNumberTypeCode, typename ScalarType> Vector<...> ADHelperScalarFunction<...>::compute_gradient() { if (ADNumberTraits<ad_type>::is_taped == true) { Assert(this->active_tape()!=this->invalid_tape_index, ExcMessage("Invalid tape index")); Assert(this->is_recording() == false, ExcMessage("Cannot compute gradient while tape is being recorded.")); Assert(this->independent_variable_values.size() == this->n_independent_variables(), ExcDimensionMismatch(this->independent_variable_values.size(),this->n_independent_variables())); TapedDrivers<ad_type,scalar_type>::gradient( this->active_tape(), this->independent_variable_values, gradient); } else { Assert(ADNumberTraits<ad_type>::is_tapeless == true, ExcInternalError()); Assert(this->independent_variables.size() == this->n_independent_variables(), ExcInternalError()); TapelessDrivers<ad_type,scalar_type>::gradient( this->independent_variables, this->dependent_variables, gradient); } }

in the helper classes makes obfuscations like

template<int dim, enum AD::NumberTypes ADNumberTypeCode, typename ScalarType> void ADHelperBase<...>::ADHelperBase( const unsigned int n_independent_variables, const unsigned int n_dependent_variables) : ... { // Tapeless mode must be configured before any active live // variables are created. if (ADNumberTraits<ad_type>::is_tapeless == true) { TapelessDrivers<ad_type,scalar_type>::initialize(n_independent_variables); } ... }

tolerable.

@bangerth Do you have any suggestions or recommendations here? I'll rebase and address all of the other comments in the mean time, but it would be great to some idea as to whether I need to address this point and that below...

I think I understand now what you are trying to do, though I'll admit that it is significantly above my head :-)

The example you gave made sense to me: In essence, you're trying to overload functions based on type, but because some of them manipulate a global environment (not an object passed in), you can't do it via function overloading -- so you do it through specialization of classes. That's a valid approach.

bangerth · 2017-11-27T05:01:41Z

include/deal.II/differentiation/ad/ad_drivers.h

+       *          a different number of fields from which a linearization must be computed.
+       */
+      static void
+      initialize (const unsigned int &n_independent_variables);


same question here about how these static member functions are supposed to work: an initialize() function must initialize something, but because the function is static it can't be an object of the surrounding type, and because the argument is const, that's not it either.

Also: no reference necessary. Same below.

bangerth · 2018-07-05T17:58:59Z

@jppelteret -- ping?

jppelteret · 2018-07-10T15:21:22Z

@bangerth Thanks for the reminder. Let me try to re-engage with you on this PR!

jppelteret · 2018-07-21T21:51:20Z

@bangerth @davydden I think that I've attended to all of the recommendations that you've made. Unfortunately ADOL-C decided to make some breaking changes between the time that this PR was initially submitted and now, so I've included some corrections for those (see the the build system changes and here). I also added some new types here which will be useful later on.

bangerth

More later.

bangerth · 2018-07-24T15:18:16Z

doc/doxygen/headers/automatic_and_symbolic_differentiation.h

@@ -408,6 +408,9 @@
 * A summary of the files that implement the interface to the supported auto-differentiable
 * numbers is as follows:
 *
+ * - ad_drivers.h: Provides classes that act as drivers to the interface of internally supported 
+ *   automatic differentiation libraries. These are used internally as an intermediary to the
+ *   the helper classes that we provide.


bangerth · 2018-07-24T15:18:54Z

doc/news/changes/minor/20171118Jean-PaulPelteret

@@ -0,0 +1,4 @@
+New: Classes that act as an interface to the drivers of internally supported automatic
+differentiation libraries (Adol-C and Sacado) have been implemented.


Is it worth referencing the AD doxygen module here? We run the changelog files through doxygen, so all of the usual markup commands and references work.

bangerth · 2018-07-24T15:31:00Z

include/deal.II/differentiation/ad/ad_drivers.h

+   * Typedef for tape indices. ADOL-C uses short integers, so
+   * we restrict outselves to similar types.
+   */
+  using tape_index_type = unsigned short;


I did not realize this earlier. This is a pretty generic name used for a pretty specific thing. Would it be possible to have these in a nested namespace types::AD, for example?

Or better, AD::types maybe?

Also, since it's in a namespace types, the suffix _type is not necessary for any of these.

Ok, I'll put this into a Differentiation::AD::types namespace.

bangerth · 2018-07-24T15:32:46Z

include/deal.II/differentiation/ad/ad_drivers.h

+  /**
+   * Typedef for tape buffer sizes.
+   */
+  using tape_buffer_type = unsigned int;


maybe reflect sizes in the type name as well?

bangerth · 2018-07-24T15:33:38Z

include/deal.II/differentiation/ad/ad_drivers.h

+#ifdef DEAL_II_WITH_ADOLC
+  // Note: This value is a limitation of ADOL-C, and not something that we
+  // have control over. See test adolc/helper_tape_index_01.cc for verification
+  // that we cannot use of exceed this value. This value is defined as TBUFNUM;


bangerth · 2018-07-24T15:52:50Z

include/deal.II/differentiation/ad/ad_drivers.h

+      static void
+      jacobian(const types::tape_index_type   active_tape_index,
+               const unsigned int &           n_dependent_variables,
+               const std::vector<ScalarType> &independent_variables,


same here

I'd almost prefer it if we called this function gradient as well.

or gradients, if you want.

I'd like to give two arguments as to why I'd like to keep this naming. First, there seems to be a genuine mathematical distinction between these two concepts. See here and here for example. Secondly, it mimics the function calls used for ADOL-C directly (they distinguish between gradient and Jacobian with the same logic as mentioned in the first point).

OK. Maybe drop the name "gradient" in the documentation of this function, and mention the jacobian() function in the documentation of the gradient() function as a cross-reference?

bangerth · 2018-07-24T15:53:17Z

include/deal.II/differentiation/ad/ad_drivers.h

+     *
+     * @tparam ADNumberType A type corresponding to a supported
+     *         auto-differentiable number.
+     * @tparam ScalarType A real or complex floating point number.


I suspect that many of the same comments from above apply here as well

bangerth · 2018-07-24T15:53:30Z

include/deal.II/differentiation/ad/ad_drivers.h

+       * @warning For ADOL-C tapeless numbers, the value given to @p n_independent_variables
+       *          should be the <b>maximum</b> number of independent variables
+       * that will be used in the entire duration of the simulation. This is
+       * important in the context of, for example, hp-FEM and for multiple


funny indentation

bangerth · 2018-07-24T15:53:42Z

include/deal.II/differentiation/ad/ad_drivers.h

+       *
+       * @warning For ADOL-C tapeless numbers, the value given to @p n_independent_variables
+       *          should be the <b>maximum</b> number of independent variables
+       * that will be used in the entire duration of the simulation. This is


in -> for the entire

same in line 356

bangerth · 2018-07-24T15:54:28Z

include/deal.II/differentiation/ad/ad_drivers.h

+      /**
+       * In the event that the tapeless mode requires <i>a priori</i> knowledge
+       * of how many directional derivatives might need to be computed, this
+       * function informs the auto-differentiable library of what this number


-> differentiation

jppelteret · 2018-07-24T20:39:19Z

@bangerth Thanks for the second round of feedback. I'll implement your suggestions tomorrow morning :-)

bangerth · 2018-07-26T07:05:43Z

As discussed, let's merge this. Can you please squash?

jppelteret · 2018-07-26T07:46:10Z

@bangerth Is it ok if I just squash the last 4 commits? Or do you want the entire PR squashed into 2 commits (8be6563 + the rest)?

bangerth · 2018-07-26T07:54:51Z

Whatever you think is appropriate. I'm just not fond of commits with commit messages such as "Address WB comments" -- they don't stand well on their own :-)

jppelteret · 2018-07-26T08:04:32Z

@bangerth Done.

bangerth · 2018-07-26T08:08:12Z

OK. Ready to merge then.

Let's wait for the indentation checker at least. Do you recall whether all tests passed before your squash?

jppelteret · 2018-07-26T08:14:24Z

I pushed an update this morning to fix a test that was still broken yesterday, so I guess that I can't say with absolute certainty that this passes the tests. I'll be patient and let the tester run on it once more.

/run-tests

bangerth · 2018-07-26T09:16:08Z

We're reasonably confident that this passed all tests, so I merged to unblock all of the follow-ups. If anything should go wrong with the tests after all, then @jppelteret will fix it on short notice.

bangerth

Here's my rest of the comments. Please address them in a separate patch if you can!

bangerth · 2018-07-26T12:48:25Z

include/deal.II/differentiation/ad/ad_drivers.h

+          {
+            trace_off(active_tape_index); // Slow
+            std::vector<std::size_t> counts(STAT_SIZE);
+            ::tapestats(active_tape_index, counts.data());


can you leave a comment about what this does? it looks like you're just passing an array of zeros (of size STAT_SIZE) here, but what does the function actually do?

or, rather, what do you do with the counts array?

I think that this should be removed (legacy of the non-descriptive tutorials I based this code off of). I'll check to see if I can legitimately remove these lines.

bangerth · 2018-07-26T12:50:33Z

include/deal.II/differentiation/ad/ad_drivers.h

+        ::function(active_tape_index,
+                   1, // Only one dependent variable
+                   independent_variables.size(),
+                   const_cast<double *>(independent_variables.data()),


groan...

can't these guys be bothered to put a const in their function declarations?

Yes, its frustrating >:-(

bangerth · 2018-07-26T12:54:16Z

include/deal.II/differentiation/ad/ad_drivers.h

+      }
+    };
+
+#  else


can you copy the condition here as a comment so a reader doesn't have to go back several hundred lines?

bangerth · 2018-07-26T12:56:31Z

include/deal.II/differentiation/ad/ad_drivers.h

+    // to provide a more descriptive error message if any of its
+    // static member functions are called.
+    template <typename ADNumberType>
+    struct TapedDrivers<


you can't call any of these functions. couldn't you have the same effect by just putting a static_assert(false, "...") into the class itself? Or is there ever a reason to have code that compiles with the non-specialized version but that is not supposed to run?

So this is to help prevent the user (and us) from having to put ifdef DEAL_II_WITH_ADOLC all over their code. The helper functions will try to use this specialisation of the class if the user has selected the type code type_code == NumberTypes::adolc_taped, and the code will always successfully compile but should never run.

but should never run

... if ADOL-C is not installed.

bangerth · 2018-07-26T12:56:46Z

include/deal.II/differentiation/ad/ad_drivers.h

+      }
+    };
+
+#  endif


copy same condition here

bangerth · 2018-07-26T13:13:28Z

include/deal.II/differentiation/ad/ad_drivers.h

+        // For reverse-mode Sacado numbers it is necessary to broadcast to
+        // all independent variables that it is time to compute gradients.
+        // For one dependent variable one would just need to all
+        // ADNumberType::Gradcomp(), but since we have a more


bangerth · 2018-07-26T13:14:01Z

include/deal.II/differentiation/ad/ad_drivers.h

+      // This function checks to see if its legal to increase the maximum
+      // number of directional derivatives to be considered during calculations.
+      // If not then it throws an error.
+      template <typename ADNumberType>


bangerth · 2018-07-26T13:15:10Z

include/deal.II/differentiation/ad/ad_drivers.h

+        else
+          {
+            // So there are some live active variables floating around. Here we
+            // check if we ask to increase the number of number of computable


number of number of

bangerth · 2018-07-26T13:15:18Z

include/deal.II/differentiation/ad/ad_drivers.h

+          {
+            // So there are some live active variables floating around. Here we
+            // check if we ask to increase the number of number of computable
+            // directional derivatives. If this really is necessary then its


bangerth · 2018-07-26T13:20:16Z

include/deal.II/differentiation/ad/ad_drivers.h

+    // forward mode to compute the first (and, if supported, second)
+    // derivatives.
+    template <typename ADNumberType, typename ScalarType>
+    struct TapelessDrivers<


bangerth · 2018-08-01T15:15:28Z

Thanks for making these last changes, @jppelteret ! If you put them into a separate PR, this should be mergeable quickly!

jppelteret added the Automatic differentiation Issues and PRs related to symbolic and automatic differentiation label Nov 18, 2017

jppelteret changed the title ~~Implement driver classes for internally supported AD libraries.~~ Implement driver interface for internally supported AD libraries. Nov 18, 2017

davydden suggested changes Nov 18, 2017

View reviewed changes

jppelteret force-pushed the ad-drivers branch from 2d2cc12 to 8afab24 Compare November 18, 2017 18:19

davydden reviewed Nov 18, 2017

View reviewed changes

jppelteret force-pushed the ad-drivers branch from 8afab24 to f667353 Compare November 18, 2017 20:13

davydden approved these changes Nov 19, 2017

View reviewed changes

jppelteret force-pushed the ad-drivers branch from f667353 to 26e5456 Compare November 19, 2017 15:37

jppelteret commented Nov 19, 2017

View reviewed changes

davydden approved these changes Nov 19, 2017

View reviewed changes

jppelteret added the WIP label Nov 21, 2017

bangerth reviewed Nov 27, 2017

View reviewed changes

jppelteret mentioned this pull request Jan 17, 2018

CMake: Implement macro to copy a file to some/all compiled test directories for one component #5525

Open

jppelteret added this to the Release 9.1 milestone Mar 4, 2018

jppelteret force-pushed the ad-drivers branch from 26e5456 to 70c890d Compare July 21, 2018 21:46

jppelteret force-pushed the ad-drivers branch 2 times, most recently from 8a0e96c to eab301f Compare July 23, 2018 15:55

jppelteret added ready for review and removed WIP ⚠️ labels Jul 23, 2018

bangerth reviewed Jul 24, 2018

View reviewed changes

jppelteret force-pushed the ad-drivers branch from 8af8b61 to fc08826 Compare July 24, 2018 22:06

jppelteret added the 6th deal.II Workshop label Jul 25, 2018

jppelteret mentioned this pull request Jul 25, 2018

AD Helpers: Introduce base class for AD helper classes #6975

Merged

4 tasks

jppelteret force-pushed the ad-drivers branch from 8b3581d to 9a55619 Compare July 26, 2018 06:15

jppelteret added 3 commits July 26, 2018 10:02

Implement driver classes for internally supported AD libraries.

e7bd499

Update configuration to detect new ADOL-C "feature" :-/

6a50c3c

Optimise and extend some of the functions in the AD drivers classes.

2c24886

jppelteret force-pushed the ad-drivers branch from 9a55619 to 2c24886 Compare July 26, 2018 08:03

jppelteret added Reviewed and ready to merge and removed ready for review labels Jul 26, 2018

bangerth merged commit b3c6ab2 into dealii:master Jul 26, 2018

bangerth reviewed Jul 26, 2018

View reviewed changes

bangerth mentioned this pull request Jul 26, 2018

Address comments in #5499 #6990

Closed

jppelteret deleted the ad-drivers branch August 1, 2018 15:16

jppelteret added this to Completed in Automatic differentiation Aug 21, 2018

		@@ -0,0 +1,4 @@
		New: Classes that act as an interface to the drivers of internally supported automatic
		differentiation libraries (Adol-C and Sacado) have been implemented.

+                    }
+                  };
+              #  else

+                    }
+                  };
+              #  endif


		}

		#ifdef DEAL_II_WITH_TRILINOS

Implement driver interface for internally supported AD libraries. #5499

Implement driver interface for internally supported AD libraries. #5499

Conversation

jppelteret commented Nov 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davydden Nov 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jppelteret commented Nov 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davydden commented Nov 19, 2017

jppelteret left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davydden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jppelteret commented Nov 19, 2017

davydden commented Nov 19, 2017

jppelteret commented Nov 20, 2017

bangerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jppelteret Jul 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangerth commented Jul 5, 2018

jppelteret commented Jul 10, 2018

jppelteret commented Jul 21, 2018

bangerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davydden Nov 18, 2017 •

edited

jppelteret Jul 10, 2018 •

edited