Skip to content
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

Implement a uniform interface to automatic differentiation libraries #5477

Closed
wants to merge 5 commits into from

Conversation

jppelteret
Copy link
Member

This commit adds the classes that give users a uniform interface to the Adol-C and Sacado automatic differentiation libraries. Also added are a selection of tests that give a indication of how the classes are used. I have many other tests that verify the actual implementation (about 27 other tests for Adol-C and 14 for Sacado), but I reckon that those are best left to another PR.

TODO list:

  • Doxygen documentation must be finished (include code snippets)
  • Documentation must be checked for relevance (it was written before I added support for Sacado)
  • Change lazy implementation of assertions (ExcInternalError) where applicable
  • Ensure that there is verification of mechanisms put in place to prevent data corruption / initialisation errors work correctly for all number types (maybe for the next PR with other tests).

@jppelteret jppelteret added Automatic differentiation Issues and PRs related to symbolic and automatic differentiation WIP labels Nov 16, 2017
@jppelteret jppelteret added this to In progress in Automatic differentiation Nov 16, 2017
Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, that's about 5000 new lines of code! I'm afraid that's beyond my ability to review for the next few weeks. Does anyone else have the necessary expertise? (Which, in fact, I don't actually have -- knowing very little about AD...)

namespace internal
{
namespace
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're in a header file -- just leave these things in the internal namespace, instead of an anonymous one

@jppelteret
Copy link
Member Author

Fair enough. I didn't realise it was quite so much. Would you prefer me to add one class at a time (there's 1 main base class and 4 derived)? Either way, I just wanted to put this out there before I go on holiday next week -- I probably couldn't attend to many comments during December anyway. I planned to spend that time extending the documentation.

@bangerth
Copy link
Member

I don't have a good solution. I just don't have the time to review such a massive amount of code, regardless of how it is packaged, in the near future :-) I'm hoping that others can look at least at parts.

@jppelteret
Copy link
Member Author

Ok, well let's collectively come up with a plan on how we can best tackle this. But tomorrow or sometime later though ;-) I'm really not in a rush to get this in since I've got plenty to keep me busy too.

@jppelteret jppelteret closed this Nov 17, 2017
@jppelteret jppelteret deleted the ad-helpers branch November 17, 2017 13:14
@jppelteret jppelteret restored the ad-helpers branch November 17, 2017 13:15
@jppelteret
Copy link
Member Author

I've decided to close this PR, and instead I'll focus on tidying up the documentation and implementation during the upcoming vacation. I'll then try to figure out how to introduce all of this in smaller chunks. I can always refer back to this closed PR to give context to what I'm doing.

@bangerth
Copy link
Member

OK. But I do think this is great work that should be merged. Please don't take my lack of time to review as lack of appreciation for your work!

@jppelteret
Copy link
Member Author

OK. But I do think this is great work that should be merged. Please don't take my lack of time to review as lack of appreciation for your work!

Thanks! I do intend on pushing forward with this. It didn't strike me that the first commit was going to be as large as it was, because in my head I have packaged these PR in the smallest logical chunks. So, I must apologise for that. I was also hoping to get some general feedback on the overall structure of the code, in case it was expected that I overhaul the implementation in some massive way.

Anyway, I'm busy stripping out some of the code into smaller pieces to be submitted individually. I'm not sure that they'll make sense without the full context, but lets try anyway :-)

@Rombur Rombur moved this from In progress to Completed in Automatic differentiation Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automatic differentiation Issues and PRs related to symbolic and automatic differentiation
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants