Skip to content
This repository was archived by the owner on Sep 9, 2024. It is now read-only.

Change ode23 to call oderkf#61

Merged
acroy merged 2 commits intoSciML:masterfrom
acroy:ode23ncm
Apr 8, 2015
Merged

Change ode23 to call oderkf#61
acroy merged 2 commits intoSciML:masterfrom
acroy:ode23ncm

Conversation

@acroy
Copy link
Copy Markdown
Contributor

@acroy acroy commented Mar 29, 2015

As discussed this changes ode23 to call oderkf with Bogacki–Shampine coefficients. The original ode23 is renamed to ode23_ncm (and moved to a separate file) for comparison and regression detection. README and LICENSE are updated.

@tomasaschan
Copy link
Copy Markdown
Contributor

LGTM, although I'm not sure why we want to keep and export the old implementation - aren't we running the same tests on the new one as we did on the old one?

Btw, the Travis build failed once due to connectivity issues with Github, probably related to the ongoing DDoS attack against GH. I restarted it hoping for better luck this time.

src/ode23_ncm.jl Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This paragraph seems wrong, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is outdated. Part of #57 was about fixing this.

@acroy
Copy link
Copy Markdown
Contributor Author

acroy commented Mar 29, 2015

It's not exported. But I also don't mind to remove it.

I also had problems to push to my github repo. Let's hope they sort it out.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 95.09% when pulling fc9b436 on acroy:ode23ncm into 8701944 on JuliaLang:master.

@acroy
Copy link
Copy Markdown
Contributor Author

acroy commented Apr 2, 2015

Thanks to @tlycken's patch I hope that both Travis runs will pass now. The question remains if we want to keep the old ode23 or not?

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 95.09% when pulling ae71674 on acroy:ode23ncm into 064a480 on JuliaLang:master.

@tomasaschan
Copy link
Copy Markdown
Contributor

Is the new implementation independent enough that we can drop any possible licensing issues we had with the old one? (I seem to recall a discussion about MATLAB's license not being MIT-compatible...)

@acroy
Copy link
Copy Markdown
Contributor Author

acroy commented Apr 2, 2015

I don't know about the form of license of the old ode23 (in LICENSE.md it only says "Copyright 2012 Cleve Moler and The MathWorks, Inc."). Our oderkf probably counts as "derived work" from Octave's ode45, which is GPL. I think it wouldn't be difficult to reimplement it (say using Hairer's book), but obviously (?) I can't do that.
Forode23s I only used the paper of Shampine & Reichelt and I put it under MIT license, which is hopefully OK.

@mauro3
Copy link
Copy Markdown
Contributor

mauro3 commented Apr 2, 2015

I that case I'd say, better remove ode23. I made an explicit RK implementation following Hairer's book. Once I've cleaned it up, I'll post it, maybe it can replace oderkf.

@tomasaschan
Copy link
Copy Markdown
Contributor

@mauro3 Nice!

@acroy To me it sounds like we don't need the old ode23 for anything anymore, so you can delete it if you like, or merge this PR as is and we'll delete it together with the current oderkf. I'm fine either way.

@acroy
Copy link
Copy Markdown
Contributor Author

acroy commented Apr 2, 2015

I removed ode23_ncm in the second commit.

@mauro3 : It would be great to have your RK implementation in ODE.jl. And it shouldn't be too hard to replace oderkf.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.59%) to 95.68% when pulling d1c0467 on acroy:ode23ncm into 064a480 on JuliaLang:master.

@ViralBShah
Copy link
Copy Markdown
Contributor

I had taken ode23 was from Cleve Moler's textbook, and ode45 from octave originally. So the comments about the licenses of both are spot on.

Perhaps it is best to remove ode23, and while GPL is perfectly fine, a non-GPL ode solver if @mauro3 has one, is also nice to have.

@tomasaschan
Copy link
Copy Markdown
Contributor

@acroy Are you waiting for something specific to merge this?

@acroy
Copy link
Copy Markdown
Contributor Author

acroy commented Apr 8, 2015

No, it was just Easter dementia :-)

acroy added a commit that referenced this pull request Apr 8, 2015
Change ode23 to call oderkf (i.e. ode23_bs) and remove old implementation.
@acroy acroy merged commit 12fc57c into SciML:master Apr 8, 2015
@acroy acroy deleted the ode23ncm branch April 8, 2015 07:50
@tomasaschan tomasaschan mentioned this pull request Apr 8, 2015
jiahao added a commit to JuliaMath/RandomMatrices.jl that referenced this pull request May 8, 2016
Now uses new ode23 interface in ODE.jl (c.f. SciML/ODE.jl#61)

Removes old demo code
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants