Skip to content

Add IDA interface callbacks #91

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

Merged
merged 17 commits into from
May 7, 2025
Merged

Add IDA interface callbacks #91

merged 17 commits into from
May 7, 2025

Conversation

alexander-novo
Copy link
Collaborator

@alexander-novo alexander-novo commented Apr 28, 2025

Description

A separate function is created for each example which needs output across different time values from IDA (until now only 1 example). This functionality should be consolidated into a single function which is general enough for all use cases/examples.

Closes #21

Proposed changes

A new callback parameter has been added to Ida::runSimulation which is called every time IDA produces a new solution.

Checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.

@rothpc
Copy link
Collaborator

rothpc commented May 1, 2025

The general idea of supporting a callback, callable by anything that understands operator() seems like a good one. I have a few comments/suggestions about the implementation.

Driven by the example of using the interface shown in commit 2f7c33d, I suggest encapsulating into a functor the declaration of the data to be stored each time step, the function that does the storing, and in this specific situation also the data structure holding the data for all time steps. In this specific example, this functor would look something like this:

struct Trace
{
    struct OutputData
    {
        double t;
        ...
    };
    vector<OutputData> output;

    operator() { output.push_back(t, yval[0], ...) }
};

To use, you'd declare a Trace object (assume it is named trace) and pass it to runSimulation(), and then access trace.output after the simulation is done to do the error check. Encapsulating these together into a functor class should make it easier for someone else to use a different functor (e.g., one that writes to a file) or just to add a new value to the data that gets saved each time step.

My second comment/suggestion is just to be aware of the scalability aspect of storing data for each time step for the entire run to a vector. For the example, I suspect it's fine because of the small amount of data saved per time step and/or a small enough number of iterations. In production, if there's more data saved or many more time steps, it might need a more complex approach that preallocates a buffer and then flushes it to disk when the buffer gets full. In the case of the example, if the number of iterations is known a priori I think I'd still use std::vector::reserve() to avoid having to reallocate and then copy its contents whenever the functor does a push_back() but the vector's current memory allocation is full.

In performance tools that save event traces (not dissimilar to what the example is doing), people deal with large data volume by using buffers that they flush to files when they get full. It's not the point of the example, so I'm not suggesting that you should implement a tracer functor that does buffer+flush here, just that there might eventually be a need for an implementation that does something more complex

@alexander-novo
Copy link
Collaborator Author

@rothpc Maybe it's worth doing what you describe for a more general class of "output capturing classes" that provide callbacks that are commonly used - such as ones that you describe here. Then rather than examples defining their own callbacks every time, they can just declare e.g. which components they care about. That sounds like it makes sense to me?

@rothpc
Copy link
Collaborator

rothpc commented May 2, 2025

I was going to suggest combining the two ideas, namely using a functor for the callback, and defining base classes for the functors that provide basic functionality. But as I thought about it - for the simpler use cases like the one shown in the example, it feels pretty artificial to declare a base class. Even the approach of providing a templatized base class that takes the struct type saved for each iteration as a template parameter - all that the base class could really do is declare the vector and a stub operator() method that would have to be overwritten. It's easier to foresee a base class for the more complicated storage situation I was talking about in my earlier comment, but an alternative approach that provides a generic (or templatized) buffering/flushing support outside of a functor, and then justing using it in functors as needed. Note also that I think the project should hold off on implementing such a thing until there's an actual need - but just be thinking about the potential for such a need in the future when designing the callback support now to hopefully going to far down a path that precludes the more complicated/scalable callback output options later.

@alexander-novo
Copy link
Collaborator Author

Yeah I think I prefer this approach in the API because it allows for your suggestion in use while also allowing for simpler lambdas in simple examples, and I'm not sure I see why I would want to force people to jump through the hoop of having to define their own class for all of this when the callback could be as simple as a single line.

@alexander-novo alexander-novo marked this pull request as ready for review May 5, 2025 20:29
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

I suggest adding updateChildern() function to aseparate branch/PR as it is out of scope here.

I also made a few other comments to discuss further.

@alexander-novo
Copy link
Collaborator Author

Should I add unit tests for this? I think I've been considering the examples as integration tests.

@alexander-novo alexander-novo requested a review from pelesh May 6, 2025 18:29
@pelesh
Copy link
Collaborator

pelesh commented May 6, 2025

Should I add unit tests for this? I think I've been considering the examples as integration tests.

Yes, a unit test would be helpful.

@alexander-novo
Copy link
Collaborator Author

Can anyone help me with this test failure? I'm just trying to create a minimal problem for IDA to solve because I'm not trying to test the solving capabilities of IDA, so I tried a 0-dim problem (which Sundials' linear solver did not like) and now I'm trying to solve the scalar problem 0=0 (I also tried the problem y' = 0 and set f_ = yp_ in evaluateResidual) but IDA is failing with a convergence error.

@pelesh pelesh mentioned this pull request May 6, 2025
6 tasks
@alexander-novo
Copy link
Collaborator Author

With Steven's advice, I updated the test problem to y = 0 instead of 0 = 0, but I'm still getting a failure.

@pelesh
Copy link
Collaborator

pelesh commented May 7, 2025

With Steven's advice, I updated the test problem to y = 0 instead of 0 = 0, but I'm still getting a failure.

I suggest we go without unit tests in this PR. The issue is tracked in #100.

@pelesh pelesh merged commit a796f48 into develop May 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GridKit interface with IDA can't return values
3 participants