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

Symbolic save_idxs first pass #2052

Closed
wants to merge 4 commits into from
Closed

Symbolic save_idxs first pass #2052

wants to merge 4 commits into from

Conversation

xtalax
Copy link
Member

@xtalax xtalax commented Jan 26, 2023

No description provided.

Copy link
Member

@YingboMa YingboMa left a comment

Choose a reason for hiding this comment

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

Should save_idxs be handled in the solver or DiffEqBase?

@xtalax
Copy link
Member Author

xtalax commented Jan 27, 2023

save_idxs is currently handled in the solver with the integrator, this is just translating symbolic indexes to integer to use that machinery, which I think makes most sense to do at problem construction time as this is the symbolic/"classic" boundary. This will also need to be handled in SciMLBase to make observed variables work again with the differing sized solution, and to fix symbolic indexing of the solution.

Do you mean that this should be a kwarg to solve rather than ODEProblem?

@xtalax
Copy link
Member Author

xtalax commented Feb 2, 2023

This is working, with the other Base PR - this is the simplest change that works, but I am open to other suggestions.

The trouble is getting the observed functions to work when they are constructed to expect all states at prob construction time, if we want to do this at solve time. This would mean allocating a dummy array that is larger than the solution to call the observed functions with, which does rather defeat the purpose of this keyword argument

@ChrisRackauckas see this for answer to your question, how to avoid dummy arrays?

e => 1.0]))
sys = structural_simplify(sys)
prob = ODEProblem(sys, [], (0, 1.0))
prob_sym = ODEProblem(sys, [], (0, 1.0), save_idxs = [a, b, d])
Copy link
Member

Choose a reason for hiding this comment

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

It would be a breaking change to move this to the problem instead of solve. Also, I don't get why it "should" be here. It makes it hard to re-solve and just save something else. Why should it be in the problem than in the solve?

Copy link
Member Author

Choose a reason for hiding this comment

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

see above for my concerns

@ChrisRackauckas
Copy link
Member

The trouble is getting the observed functions to work when they are constructed to expect all states at prob construction time, if we want to do this at solve time. This would mean allocating a dummy array that is larger than the solution to call the observed functions with, which does rather defeat the purpose of this keyword argument

If you do it in the solver though, how are you doing the interpolations?

@xtalax
Copy link
Member Author

xtalax commented Feb 5, 2023

I thought that the interpolations were over time rather than idx, so we'd be able to use the same setup. When are the interpolations constructed?

@ChrisRackauckas
Copy link
Member

They are constructed during the solve, but only for the states. So if you're planning to extend the u, how are you planning to extend the k?

@YingboMa
Copy link
Member

YingboMa commented Feb 5, 2023

Only solvers like BDF can handle that, because the underlaying interpolant doesn't depend on f at all.

@xtalax
Copy link
Member Author

xtalax commented Feb 5, 2023

They are constructed during the solve, but only for the states. So if you're planning to extend the u, how are you planning to extend the k?

My apologies, I'm not familiar with the shorthand k, what are you referring to?

@YingboMa
Copy link
Member

YingboMa commented Feb 5, 2023

ks are the stage derivatives in the Runge-Kutta setting https://en.wikipedia.org/wiki/Runge%E2%80%93Kutta_methods#Explicit_Runge%E2%80%93Kutta_methods

They are required to form the dense output. See section 4 of https://www.sciencedirect.com/science/article/pii/S0898122111004706

@xtalax
Copy link
Member Author

xtalax commented Feb 5, 2023

The states are not in general linked in any kind of spatial configuration during an ODE solve, and appear only to be linked by the calculation of the truncation error in that paper.

I am unsure whether it is possible to ignore the contribution of the unsaved states to the truncation error, but regardless this is the only thing that might be changed by changing the saved states, unless I'm missing something.

Meaning I don't think that the calculation of the k would change, except from possibly dropping unnecessary states from the calculation

@ChrisRackauckas
Copy link
Member

But if you make save_idxs have an observable, how do you extend k? That's something we haven't worked out yet.

@xtalax
Copy link
Member Author

xtalax commented Feb 5, 2023

save_idxs having an observable is at the moment just automatically saving its state dependencies as a first pass

@xtalax
Copy link
Member Author

xtalax commented Feb 17, 2023

superseded by #2071

@xtalax xtalax closed this Feb 17, 2023
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.

None yet

3 participants