Skip to content

Conversation

@kanav99
Copy link
Contributor

@kanav99 kanav99 commented Aug 20, 2019

Credits @devmotion. I just took the resize implementation from #315 as I explained in #325 (comment)

@devmotion
Copy link
Member

As I mentioned in #325 (comment), I think we should reconsider the order in which we want to add changes. The PR from which the resize! implementation is taken is outdated and should be closed. So IMO it feels wrong to add changes from that PR.

@kanav99
Copy link
Contributor Author

kanav99 commented Aug 25, 2019

Even though the PR was closed, I dont think considering this resize is bad, its more extendible more clean.

@devmotion
Copy link
Member

I agree that it's better than the current version but I don't see in what way it would be better than the changes in #325? On the contrary, I think the changes in #325 are superior since resize!(::NLSolver, ::Int) does not consider any special cases for NLAnderson anymore. Since hopefully the registry will be updated soon with the correctly upper bounded DiffEqBase dependencies for OrdinaryDiffEq and StochasticDiffEq, I think the main goal should be to agree on a final version of #325.

IMO, by merging this PR we do not gain much at the moment, it seems it would mostly be helpful for SciML/OrdinaryDiffEq.jl#878. I'm pretty sure we would have to rebase #325, which provides similar but slightly improved resize changes.

@ChrisRackauckas
Copy link
Member

Looks like we are going straight to the newer nlsolvers.

@ChrisRackauckas ChrisRackauckas deleted the resize branch August 31, 2019 21:04
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.

5 participants