Implemented non_linear_time, an array of time as an input to step function

[prady0t]

Description

Fixes #3524

Now an array of time can be passed to step function which will return solutions at that step times.
Example:

step_solver.step(step_solution, model, non_linear_time=[0, 1000, 1500, 1700, 2000])

Output:

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (8056d22) to head (8afcea3).
Report is 3 commits behind head on develop.

❗ Current head 8afcea3 differs from pull request most recent head a1ac193. Consider uploading reports for the commit a1ac193 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3627   +/-   ##
========================================
  Coverage    99.60%   99.60%           
========================================
  Files          259      259           
  Lines        21273    21275    +2     
========================================
+ Hits         21189    21191    +2     
  Misses          84       84           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arjxn-py]

Thanks @prady0t, looks good. Happy to see almost all tests passing, although to fix coverage you might want to modify test_base_model relative to your changes. Requesting @rtimms review for further refinements.

[rtimms]

Thanks for taking this on. I don't think it is necessary to have both npts and non_linear_time. Instead, I think we should deprecate npts, require users to pass dt and make t_eval an optional argument.

In the deprecation warning we can explain that you can calculate t_eval = np.linspace(t_start, t_end, npts) and pass that instead.

That way, you don't need to have repeated logic depending on which optional argument is passed, and the API is a bit cleaner - better to have only one way of doing things. It would also mimic solve_ivp's API where you pass t_span (the start and end time) and optionally t_eval.

tagging @TomTranter as I know you use step a fair bit in other projects

[TomTranter]

Thanks for taking this on. I don't think it is necessary to have both npts and non_linear_time. Instead, I think we should deprecate npts, require users to pass dt and make t_eval an optional argument.

In the deprecation warning we can explain that you can calculate t_eval = np.linspace(t_start, t_end, npts) and pass that instead.

That way, you don't need to have repeated logic depending on which optional argument is passed, and the API is a bit cleaner - better to have only one way of doing things. It would also mimic solve_ivp's API where you pass t_span (the start and end time) and optionally t_eval.

tagging @TomTranter as I know you use step a fair bit in other projects

@rtimms suggestions sound sensible to me

[prady0t]

Thanks for taking this on. I don't think it is necessary to have both npts and non_linear_time. Instead, I think we should deprecate npts, require users to pass dt and make t_eval an optional argument.

In the deprecation warning we can explain that you can calculate t_eval = np.linspace(t_start, t_end, npts) and pass that instead.

That way, you don't need to have repeated logic depending on which optional argument is passed, and the API is a bit cleaner - better to have only one way of doing things. It would also mimic solve_ivp's API where you pass t_span (the start and end time) and optionally t_eval.

tagging @TomTranter as I know you use step a fair bit in other projects

Yes that makes much more sense! Making changes now along with code coverage.

[prady0t]

Should I change name of variable non_linear_time to something more accurate or is it ok?
Also how to handle logic when function has both dt and non_linear_time as input? Should I just calculate based on non_linear_time and ignore dt in that case? Whats the use of dt if we already have non_linear_time? Am I missing some logic here? @rtimms

[rtimms]

You should always have to pass dt, and t_eval should be the optional argument. If t_eval is None then set t_eval = np.linspace(0, dt, 2) (this is the same as the default behaviour before with npts=2). If t_eval is not None then check that t_eval[0] = 0 and t_eval[-1]=dt.

[valentinsulzer]

Why not just require t_eval (and no dt) so that the API matches solve ?

[rtimms]

Why not just require t_eval (and no dt) so that the API matches solve ?

Happy with this too, but step was kind of intended to be "step this model forward in time this amount", so having dt makes sense in that context.

[prady0t]

Now the output of

while time < end_time:
    step_solution = step_solver.step(step_solution, model, dt=500, non_linear_time=[0, 50, 100, 200, 500])
    time += dt

Looks like:

[prady0t]

Should we also remove npts from here ? @rtimms @tinosulzer

[valentinsulzer]

Yes

[prady0t]

I've made the changes! Please have a look.

[rtimms]

thanks @prady0t looks great! can you add a changelog entry then this is good to go

[prady0t]

@agriyakhetarpal why is this CI job failing?

[kratman]

@prady0t that check has been failing for a while. If you did not add any links in your PR then it is probably unrelated. I did not check if you made any URL changes, but I know it was failing before due to stuff in the all-contributors section and other markdown files.

The check looks to see if it can access the URLs found in the codebase, and fails if any of them are unreachable. The idea is that if we put a link to a documentation page, it warns us if the URL stops working. Just check the links in the failing test to make sure it is not from anything you did. If it is unrelated then we will ignore it

[kratman]

@rtimms Is anything left for your review of this PR?

[kratman]

@prady0t I will merge this when CI passes. I think everything is done

Changes appear to have been completed