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

Contributing with examples #87

Merged
merged 1 commit into from
Mar 3, 2016
Merged

Contributing with examples #87

merged 1 commit into from
Mar 3, 2016

Conversation

pjpmarques
Copy link
Contributor

Happy to contribute with a few jupyter notebooks on how to use the package.
You can check the examples/result here:

* [Simple differential equation](http://nbviewer.jupyter.org/github/pjpmarques/ODE.jl/blob/master/examples/Simple_Differential_Equation.ipynb)
* [Lorenz Attractor](http://nbviewer.jupyter.org/github/pjpmarques/ODE.jl/blob/master/examples/Lorenz_Attractor.ipynb)
* [Terminal Velocity](http://nbviewer.jupyter.org/github/pjpmarques/ODE.jl/blob/master/examples/Terminal_Velocity.ipynb)

Copy link
Contributor

Choose a reason for hiding this comment

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

These links would need to point to this repo and not your fork.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be (but isn't necessarily) nice to first merge this, though, and then when the notebooks exist in ODE.jl, refer to them with a specific hash rather than master. This would ensure that links aren't broken by mistake if/when the notebooks are (re)moved, but on the other hand it would require manual update of all links when the notebooks are changed, even if the location did not change; and furthermore, that manual change would not be possible to make before the changes to the notebooks have been committed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally the notebooks would be checked with each commit. Is this possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Checked" as in "run-through with no errors"?

No idea - I doubt that it's easy. It would be a nice feature of IJulia, though, so if it doesn't exist it's worth a feature request there. I imagine something like this:

using IJulia
run_all_cells("path/to/notebook.ipynb")

and as a first step the test would just verify that running the above code doesn't throw. (More features could include checking expected output from each cell, checking expected STDOUT printouts etc, but just running without crashing is a good start.)

@mauro3
Copy link
Contributor

mauro3 commented Feb 24, 2016

Thanks for these examples! I think the would be good to have in ODE.jl.

At the moment there are no docs for ODE.jl. But one thing to ponder is how we'd like to have examples. Are notebooks a suitable format? However, if the answer is "no", then these could be ported over.

A few comments for the Lorenz notebook (for some reason, I cannot do them inline):

  • put a link to wikipedia in the intro?
  • remove "Let's implement it in Julia."
  • make your global sigma, rho, bet all const
  • any reason to not use ode45?

Also, squash the commits into one. And, I think, the by Paulo Marques, 2014/01/28 should be removed.

@pjpmarques
Copy link
Contributor Author

Hi. Glad to correct the code and follow the suggestions. I've:

  • Solved the links problem pointing to outside repository.
  • Marked constants with "const"
  • Gave credit to Wikimedia and include references to wikipedia
  • Changed ode23() to ode45()
  • Removed personal credit
  • Corrected some incorrect notation (use of == instead of =; use of YY instead of Y)
  • Use of less colloquial style
  • Squashed all commits into one

Note that now the links will appear to be broken because they are pointing to ODE.jl/master, which still doesn't contain this code. It will only render properly after the merge.

@pjpmarques
Copy link
Contributor Author

Hi. Any news on this and/or do you want me to correct something? Thanks.

@mauro3
Copy link
Contributor

mauro3 commented Mar 3, 2016

LGTM

ivarne added a commit that referenced this pull request Mar 3, 2016
Contributing with examples
@ivarne ivarne merged commit c12fc36 into SciML:master Mar 3, 2016
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.

4 participants