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

Retry add ivp solver #71

Merged
merged 54 commits into from
Aug 31, 2014
Merged

Conversation

davidrpugh
Copy link
Contributor

@spencerlyon2 Hopefully we are all set!

@sglyon
Copy link
Member

sglyon commented Aug 29, 2014

This looks great!! How did you get 54 of these commits in?

@jstac, once travis responds that all is well I think this should (finally 😄) be ready to merge.

@sglyon sglyon mentioned this pull request Aug 29, 2014
@davidrpugh
Copy link
Contributor Author

Not sure where all of that history came from...I checked out the most recent version of the three files from add-ivp-solver and along came some history (mostly related to files that are not being merged!).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.24%) when pulling 9041acb on davidrpugh:retry-add-ivp-solver into beca26e on jstac:master.

@davidrpugh
Copy link
Contributor Author

@spencerlyon2 As far as I am concerned this is ready for a merge. I could always tinker with the examples notebook, but I have probably reached the point of diminishing returns. Does @jstac need to weigh in?

@sglyon
Copy link
Member

sglyon commented Aug 29, 2014

@davidrpugh, yes I agree. This is ready to go.

We will wait for @jstac to take a look, but I imagine this will be merged soon.

@jstac
Copy link
Contributor

jstac commented Aug 29, 2014

@davidrpugh @spencerlyon2

@jstac
Copy link
Contributor

jstac commented Aug 29, 2014

Let's try that again

@davidrpugh @spencerlyon2

Thanks for the heroic effort. I hope you at least learned some useful stuff with all that wrestling with git.
I'll work through this at some stage today or tonight and if all is good I'll accept the PR.

@jstac
Copy link
Contributor

jstac commented Aug 31, 2014

@davidrpugh

This all looks great. The notebook is excellent. The exposition is very nice now. We'll feature it on the new QuantEcon site when it goes up in a week or two. I'll go ahead and merge. The following are suggestions for small improvements that you can implement post-merge or ignore as you see fit:

  • You mention that an ODE is a functional equation and talk about "the" solution, and then go on and discuss the solution to the IVP. It might be worth pointing out the difference between the two --- the former problem has many solutions and the IVP picks out one.
  • You might want to remove the derivative sign from (1.3) and put in an explicit difference --- especially since you describe it as a "difference equation".
  • You are welcome to add university logos to the notebook (I think this came up earlier). This notebook is separate to the official documentation on readthedocs.

jstac added a commit that referenced this pull request Aug 31, 2014
@jstac jstac merged commit e9b5bf0 into QuantEcon:master Aug 31, 2014
@davidrpugh davidrpugh deleted the retry-add-ivp-solver branch February 10, 2015 09:12
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.

4 participants