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: QNDF mass matrix tests #1094

Merged
merged 6 commits into from Mar 28, 2020
Merged

Conversation

utkarsh530
Copy link
Member

Fixes All BDF and NDF methods for mass matrix tests #1077.
@ChrisRackauckas please review.

@utkarsh530 utkarsh530 changed the title Fix: QNDF mass_matrix_tests Fix: QNDF mass matrix tests Mar 26, 2020
@kanav99
Copy link
Contributor

kanav99 commented Mar 26, 2020

Split Regression test set into two sets

@@ -653,8 +653,6 @@ function perform_step!(integrator,cache::QNDFConstantCache,repeat_step=false)
end
backward_diff!(cache,D,D2,k)
end
else
Copy link
Member

Choose a reason for hiding this comment

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

what happens during the else now?

Copy link
Member Author

Choose a reason for hiding this comment

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

The gamma value should not be updated before cnt>k, the code was doing that previously. You can see the same results in QNDF1. (Current Implementation gives 1st order estimate only which needs to be checked) Even the norm value is same as QNDF1

@@ -327,7 +327,7 @@ function alg_cache(alg::QNDF,u,rate_prototype,uEltypeNoUnits,uBottomEltypeNoUnit

max_order = 5
atmp = similar(u,uEltypeNoUnits)
utilde = nlsolver.tmp
utilde = similar(u)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
utilde = similar(u)
utilde = zero(u)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay

@utkarsh530
Copy link
Member Author

@ChrisRackauckas made the requested changes.

@ChrisRackauckas
Copy link
Member

Tests fail

@utkarsh530
Copy link
Member Author

Rebuiliding Travis

@utkarsh530 utkarsh530 closed this Mar 26, 2020
@utkarsh530 utkarsh530 reopened this Mar 26, 2020
@kanav99
Copy link
Contributor

kanav99 commented Mar 27, 2020

If you compare the times of all jobs in last master successful build here and this build, it seems like Travis has cut down it's resources recently and every job has its time increased. I think the best bet is to split up the Regression tests.
I am restarting the master tests if my claim is right.

@utkarsh530
Copy link
Member Author

utkarsh530 commented Mar 27, 2020

Alright @kanav99 thanks for the help. I will update the PR with split regression tests.
Update: https://travis-ci.org/github/SciML/OrdinaryDiffEq.jl/builds/665110567?utm_source=github_status&utm_medium=notification
master is failing too. @ChrisRackauckas @kanav99 I could split the regression tests as how Kanav is saying and make another PR?

@utkarsh530
Copy link
Member Author

@ChrisRackauckas please have a look.

@codecov
Copy link

codecov bot commented Mar 27, 2020

Codecov Report

Merging #1094 into master will increase coverage by <.01%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1094      +/-   ##
==========================================
+ Coverage   77.33%   77.33%   +<.01%     
==========================================
  Files          95       95              
  Lines       31656    31659       +3     
==========================================
+ Hits        24481    24485       +4     
+ Misses       7175     7174       -1
Impacted Files Coverage Δ
src/caches/bdf_caches.jl 88.65% <100%> (ø) ⬆️
src/perform_step/bdf_perform_step.jl 88.71% <90%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59767c4...b286dd9. Read the comment docs.

@ChrisRackauckas
Copy link
Member

These fixes will do more than just mass matrices: check the benchmarks and see if they improved.

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.

None yet

3 participants