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

Fixed up the Nelson ODE function #7

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fixed up the Nelson ODE function #7

wants to merge 6 commits into from

Conversation

nnd389
Copy link

@nnd389 nnd389 commented Oct 25, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Hello! My name is Nina De La Torre, and I wrote this ODE function! It was such a pleasant surprise to see it on the Julia documentation website, but the version that was used was unfinished, and I wanted to fix it up a bit. Anyways, here is the same function but a bit nicer to look at, with helpful comments, parameters, and a cute name. I am planning to request updates the other nelson files as well. I hope this is helpful!

Kindly,
Nina

@ChrisRackauckas
Copy link
Member

Hi! Stella showed me this example to turn it into a benchmark, but of course I don't know the model all that much so thank you for your contributions.

Note that this repo is the website output files, but it's generated by https://github.com/SciML/SciMLBenchmarks.jl. Thus the files here shouldn't be updated directly, instead https://github.com/SciML/SciMLBenchmarks.jl/blob/master/benchmarks/AstroChem/nelson.jmd should be updated and the CI will automatically generate the results from the script. Could you make a PR to the generator with these changes?

Comment on lines +166 to +168
sol2 = solve(prob, FBDF())
sol3 = solve(prob, QNDF())
sol4 = solve(prob, CVODE_BDF())
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for using the high tolerances here? That's almost certainly the reason why they are different from the reference.

@nnd389
Copy link
Author

nnd389 commented Oct 25, 2024 via email

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