Feature/oo adaptivity #289
Conversation
…routine is identical
…DE. Code builds, but still needs work/debugging.
… use default SUNControl implementation.
…uristics name change issues in various F2003 interface files
…SUNAdaptController structures
…Heuristics. Adjusted SUNAdaptController API according to PR discussion.
|
I believe that I've addressed all PR comments (modulo some answers I provided to questions that may require further discussion, and modulo changing any "default" behavior as Steven requested). The automated tests are currently failing because of differences in runtime statistics (some improved, others not). While I work through those test result differences, I would appreciate it if @gardner48, @balos1, and @Steven-Roberts could look back through this PR to see if their previous requests/questions have been satisfied, and to see if my new changes pass their approval. |
| The enumerated type :c:type:`SUNAdaptController_Type` defines the enumeration | ||
| constants for SUNDIALS error controller types | ||
|
|
||
| .. c:enumerator:: SUN_ADAPTCONTROLLER_NONE |
There was a problem hiding this comment.
Is this type necessary? Can't we implement a SUN_ADAPTCONTROLLER_H that does not change the value of h, i.e., an identity controller.
There was a problem hiding this comment.
Did you happen to apply clang-format to all of the new source files? If not, we should going forward, but I will update these later.
There was a problem hiding this comment.
I did not run clang-format.
|
|
||
| typedef enum | ||
| { | ||
| SUN_TIMESTEPHEURISTICS_STD, |
There was a problem hiding this comment.
Do we prefer to call them standard, or default? I think this should match how we talk about them in the docs.
There was a problem hiding this comment.
Looks like the implementation is called "default", so we should change this enum name to "default".
|
|
||
| SUNAdaptController_Type SUNAdaptController_GetType(SUNAdaptController C) | ||
| { | ||
| if (C == NULL) { return SUN_ADAPTCONTROLLER_NONE; } |
There was a problem hiding this comment.
I think we need to be very careful about purposefully making NULL to be a valid object, which is what we seem to be doing here. This could lead to some hard to debug issues I imagine. I think the better approach would be to implement a controller that does not change the step size (as I recommended in a different comment).
|
|
||
| SUNTimestepHeuristics_ID SUNTimestepHeuristics_GetID(SUNTimestepHeuristics H) | ||
| { | ||
| if (H == NULL) { return SUN_TIMESTEPHEURISTICS_NULL; } |
There was a problem hiding this comment.
Same comment as on the controller. I dont think we want to make NULL a valid "object."
…ut had forgotten to add 1 to the embedding order
|
Closing this PR, and replacing with #358 |
New object-oriented infrastructure for time step adaptivity controllers, and time step heuristic constraints. These are currently used in ARKODE. In the not-distant future, we can implement additional step/order controllers within this existing infrastructure to support CVODE(S) and IDA(S) (and maybe even ARKODE).
Rendered docs can be viewed here: https://sundials.readthedocs.io/en/feature-oo-adaptivity/index.html