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

fix vector interpolation and remove unnecessary @inbounds #478

Merged

Conversation

oscardssmith
Copy link
Contributor

@oscardssmith oscardssmith commented Aug 10, 2023

This is a followup to #476 that fixes some typos and also fixes a bounds error. This also removes unnecessary @inbounds that don't improve performance.

Benchmark:

julia> using Sundials

julia> mutable struct precflags
           prec_used::Bool
           psetup_used::Bool
       end

julia> prob = DAEProblem(prob_dae_resrob.f, prob_dae_resrob.du0, prob_dae_resrob.u0,
           prob_dae_resrob.tspan,  precflags(false, false));

julia> sol = solve(prob, IDA());

#before
julia> @btime sol(85000)
  3.474 μs (55 allocations: 4.67 KiB)

julia> @btime sol(1000:1000:90000);
  283.921 μs (4776 allocations: 420.81 KiB)

#after
julia> @btime sol(85000)
  3.450 μs (55 allocations: 4.67 KiB)
  
julia> @btime sol(1000:1000:90000);
  284.034 μs (4776 allocations: 420.81 KiB)

Copy link

@ai-maintainer ai-maintainer bot left a comment

Choose a reason for hiding this comment

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

AI-Maintainer Review for PR - Fix vector interpolation and remove unnecessary @inbounds

Title and Description 👍

The Title and description are clear, concise and helpful
The title and description of the pull request are clear and concise. They effectively communicate the purpose of the changes, which is to fix vector interpolation and remove unnecessary `@inbounds`. The description also provides additional context by mentioning that it is a follow-up to a previous pull request, fixes some typos, and addresses a bounds error.

Scope of Changes 👍

The changes are narrowly focused
The changes appear to be narrowly focused on fixing vector interpolation and removing unnecessary `@inbounds`. There is no indication that the author is trying to resolve multiple issues simultaneously.

Testing ⚠️

Testing details are missing
The description does not mention how the author tested the changes. It is recommended to include details about the testing process to ensure the changes work as expected and do not introduce new issues.

Code Changes 👍

Code changes are appropriate and well-executed
The code changes are appropriate and seem to be well-executed. The removal of `@inbounds` is justified as it does not improve performance. The changes in the interpolation logic appear to fix the mentioned issues. However, it would be beneficial to have more context or comments explaining the logic, especially for contributors who are not familiar with the codebase.

Recommendations

  • Please provide details on how the changes were tested. This could include unit tests, integration tests, or manual testing steps.
  • Consider adding comments to complex code blocks to improve readability and maintainability.

Reviewed with AI Maintainer

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #478 (5c619ce) into master (0bffe3e) will decrease coverage by 1.63%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #478      +/-   ##
==========================================
- Coverage   56.28%   54.65%   -1.63%     
==========================================
  Files          47       47              
  Lines        3587     3586       -1     
==========================================
- Hits         2019     1960      -59     
- Misses       1568     1626      +58     
Files Changed Coverage Δ
src/interpolation.jl 33.20% <50.00%> (ø)

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ChrisRackauckas ChrisRackauckas merged commit cce1ce0 into SciML:master Aug 10, 2023
53 of 61 checks passed
@oscardssmith oscardssmith deleted the fix-interpolation-bounds-followup branch August 10, 2023 22:03
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