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 copy! for noise types #127

Merged
merged 28 commits into from
Oct 22, 2022
Merged

Conversation

rmsrosa
Copy link
Contributor

@rmsrosa rmsrosa commented Oct 19, 2022

Ok, I am opening this PR so we can discuss whether this approach and the implementation are ok or not and, hopefully, get it approved. I've been scratching my head over this for the past few days.

There is this issue to connect prob.noise with sol.W when solving an SDE, RODE, etc.. We need, somehow, to let prob know that we are solving the problem again, so a new reseed! and a new reinit! can be triggered and a different sample path can be used.

Originally, we had prob.noise = sol.W, which had the problem of affecting all the previous solves because they all alias sol.W. My first fix was to deepcopy the prob.noise when solving it and then deepcopy the sol.W back to prob.noise (SciML/StochasticDiffEq.jl#496). This week, however, I found a side effect of that, which is that of dealiasing sol.W.u from sol.W.W. And this also has the problem of abusing deepcopy. So, there is this idea of adding a proper copy for noise types, to avoid using deepcopy. This is an attempt of that.

  1. I implemented copy by recursively going down the fields and either assigning or using copy or recursivecopy or, at the last resort, deepcopy. Everything seems fine, but I can't say I am 100% sure it won't break at some point. But that is how it usually is. In all tests, here and over at StochasticDiffEq, it seems to be deep copying only the source fields of very specific noise types such as NoiseWrapper and NoiseApproximation, some of which actually use deepcopy themselves, upon creation of an instance. So I guess that is okay. My only concern is that some types are subtypes of AbstractArray and could get down to recursivecopy and end up not being copied properly or failing. But I haven't found any case of that so far. (if one tries to recursivecopy a VirtualBrownianTree, it breaks, but my implementation of copy seems fine).
  2. I implemented an equality check for noises but that is only for testing. It checks most fields but not all since the equality is not implemented for integrators and some other stuff and it would be quite an effort to check that. But it checks many more fields than the equality check inherited from AbstractArray. Because of that, I used a different symbol, in part not to mess with other tests (extraction_test.jl compares sol and _sol and there are two fields that are different, like sol.curt, if I remember correctly, and a proper equality would fail.)
  3. Is it ok to call it copy and copy! since it might eventually use deepcopy?

@rmsrosa
Copy link
Contributor Author

rmsrosa commented Oct 20, 2022

Some more thoughts:

  1. The current PR only affects solving a NoiseProblem, because it is the only place here that uses W = copy(prob.noise).
  2. copy! will be used in StochasticDiffEq.jl only if the new PR Use copy for noise StochasticDiffEq.jl#509 is merged. Deving shows everything works fine in the local tests. But, alternatively, one can keep using deepcopy in StochasticDiffEq.jl and then do W.u = W.W after W = deepcopy(prob.noise), in the beginning of __solve, and then prob.noise.u = prob.noise.W after deep copying integrator.sol.W back to prob.noise, at the end of __solve to eliminate the bug.

But my understanding is that there is something odd about noise types. Typically, there is a clear distinction between a problem, its arguments, and its solution. For instance, an ODEProblem only contains information about the initial value problem, and solve gives the solution path. SDEProblem defines a stochastic problem and solve gives a sample path. Actually SDEProblem is not a pure stochastic problem because it is "polluted" with the information of the sample path from the noise part of the problem. A noise type, in turn, serves both the purpose of defining a stochastic process and containing a sample path. And this is part of the problem. I think one long-term solution would be to have a noise type only contain information about the distribution of a noise; have a NoiseProblem only contain the associated information of the stochastic process, and consequently have SDEProblem only contain the information of its stochastic process; and then define a new solution type for NoiseProblem and which would also go to sol.W when solving an SDEProblem. And then only this solution type would have information about a sample path of a noise process.

@ChrisRackauckas
Copy link
Member

I think one long-term solution would be to have a noise type only contain information about the distribution of a noise; have a NoiseProblem only contain the associated information of the stochastic process, and consequently have SDEProblem only contain the information of its stochastic process; and then define a new solution type for NoiseProblem and which would also go to sol.W when solving an SDEProblem. And then only this solution type would have information about a sample path of a noise process.

Yes, I've thought about that too, but it seemed like a really big lift and it's hard to know what the tangible advantage in the end ends up being.

@ChrisRackauckas ChrisRackauckas merged commit 0b5de32 into SciML:master Oct 22, 2022
@rmsrosa
Copy link
Contributor Author

rmsrosa commented Oct 23, 2022

Yes, I've thought about that too, but it seemed like a really big lift and it's hard to know what the tangible advantage in the end ends up being.

Yeah, I understand that. It is not clear the advantages are worth the trouble. But I will keep this in mind to maybe try implementing this at some point, in a nonbreaking way, adding fields and types instead of replacing them, for testing purposes.

@rmsrosa rmsrosa deleted the copy_noise branch November 3, 2022 09:46
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.

2 participants