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.

Sign up for GitHub

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

Jump to bottom

Fix kinsol #12254

Merged
merged 3 commits into from
May 21, 2021
Merged

Fix kinsol #12254

merged 3 commits into from
May 21, 2021

Conversation

luca-heltai
Copy link
Member

@luca-heltai luca-heltai commented May 19, 2021 •
edited

This should fix all remaining issues with KINSOL.

closes #12223.

@luca-heltai
Copy link
Member Author

@sebproell , this addresses everything we discussed on slack. Thanks to you and to @nicola-giuliani for the help.

@peterrum
Copy link
Member

/rebuild

bangerth
bangerth reviewed May 19, 2021
View reviewed changes
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.

I have just one question, but the rest works for me.

I didn't see where the original bug was. To me it looks like you are only shuffling code into the right place, and support a couple of things that may previously not have been supported (like the get_constraints() functionality). What was the bug and which part fixes it?

source/sundials/kinsol.cc
status = KINSetNumMaxIters(kinsol_mem, data.maximum_non_linear_iterations);
AssertKINSOL(status);

// From the manual: this must be called BEFORE KINInit
Copy link
Member

Choose a reason for hiding this comment

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

how bizarre...

Copy link
Contributor

Choose a reason for hiding this comment

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

@bangerth this was the bug. @sebproell discovered that if this function is called after the initialisation a vector of N_Vector is left empty so the fixed point iterations end up segfaulting. Then we looked carefully at the manual and discovered the fix

source/sundials/kinsol.cc Outdated
@@ -524,6 +542,8 @@ namespace SUNDIALS
reinit_vector = [](VectorType &) {
AssertThrow(false, ExcFunctionNotProvided("reinit_vector"));
};

setup_jacobian = [](const VectorType &, const VectorType &) { return 0; };
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't you want this function to throw an assert, just like 3 lines higher up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. We claim it is optional. Indeed the older version worked without this method. The new version, however, requires the setup. Providing a "do-nothing" setup by default should maintain backward compatibility and allow one not to specify any setup if required.

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case, can you please move this line out of this function? The function is called set_functions_to_trigger_an_assert() :-)

I think the place where we do the general KINSOL set-up would be right.

@bangerth
Copy link
Member

The CI check shows this failure:


[ 33%] Generating kinsol_02.debug/interrupt_guard.cc
Scanning dependencies of target sundials.kinsol_02.debug
[ 66%] Building CXX object CMakeFiles/sundials.kinsol_02.debug.dir/kinsol_02.cc.o
[ 50%] Building CXX object CMakeFiles/sundials.kinsol_02.debug.dir/kinsol_02.debug/interrupt_guard.cc.o
[ 83%] Linking CXX executable kinsol_02.debug/kinsol_02.debug
[166%] Built target sundials.kinsol_02.debug
Scanning dependencies of target sundials.kinsol_02.debug.diff
[216%] Generating kinsol_02.debug/output
sundials/kinsol_02.debug: BUILD successful.
sundials/kinsol_02.debug: RUN failed. ------ Return code 134
sundials/kinsol_02.debug: RUN failed. ------ Result: /home/dealii/build/tests/sundials/kinsol_02.debug/failing_output
sundials/kinsol_02.debug: RUN failed. ------ Partial output:

sundials/kinsol_02.debug: RUN failed. ------ Additional output on stdout/stderr:

terminate called after throwing an instance of 'std::bad_function_call'
  what():  bad_function_call

Can you take another look?

@luca-heltai
Copy link
Member Author

I'll be looking at the failure later today. It is happening in the old version of sundials, not the new one.

@bangerth
Copy link
Member

Looks good to me, and it also tests correctly, but I would still like it if you could address this one comment: https://github.com/dealii/dealii/pull/12254/files#r636201155

@bangerth
Copy link
Member

OK to merge with this one change.

@luca-heltai
Copy link
Member Author

see my comment in #12200. Should we rename the function in this class, or the ones in IDA?

@bangerth
Copy link
Member

Let's get this one merged as-is and leave the renaming for later.

@bangerth
Copy link
Member

later = later today, in a separate patch

@luca-heltai luca-heltai merged commit 0611f1b into dealii:master May 21, 2021
@bangerth bangerth mentioned this pull request Jul 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicola-giuliani nicola-giuliani nicola-giuliani left review comments

@bangerth bangerth bangerth approved these changes

@peterrum peterrum peterrum approved these changes

Assignees
No one assigned
Labels
ready to test Reviewed and ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KINSOL not working
4 participants
@luca-heltai @peterrum @bangerth @nicola-giuliani