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

ci: update tests workflow to use centralized reusable workflow and auto-suggest formatting changes on PRs #204

Merged
merged 30 commits into from
Aug 17, 2024

Conversation

thazhemadam
Copy link
Member

Follow up to #183.

Copy link
Contributor

github-actions bot commented Jul 31, 2024

Benchmark Results

master ba4a3a5... master/ba4a3a51af4fa9...
Simple Pendulum/IIP/BoundaryValueDiffEq.MIRK2() 6.52 ± 0.21 ms 6.57 ± 0.23 ms 0.993
Simple Pendulum/IIP/BoundaryValueDiffEq.MIRK3() 2.35 ± 0.093 ms 2.36 ± 0.081 ms 0.995
Simple Pendulum/IIP/BoundaryValueDiffEq.MIRK4() 0.822 ± 0.033 ms 0.825 ± 0.033 ms 0.996
Simple Pendulum/IIP/BoundaryValueDiffEq.MIRK5() 0.875 ± 0.038 ms 0.874 ± 0.037 ms 1
Simple Pendulum/IIP/BoundaryValueDiffEq.MIRK6() 0.988 ± 0.036 ms 0.985 ± 0.034 ms 1
Simple Pendulum/IIP/MultipleShooting(10, Tsit5; grid_coarsening = false) 1.34 ± 0.11 ms 1.33 ± 0.12 ms 1.01
Simple Pendulum/IIP/MultipleShooting(10, Tsit5; grid_coarsening = true) 2.59 ± 0.21 ms 2.6 ± 0.2 ms 0.994
Simple Pendulum/IIP/MultipleShooting(100, Tsit5; grid_coarsening = false) 0.0362 ± 0.0057 s 0.0362 ± 0.005 s 1
Simple Pendulum/IIP/MultipleShooting(100, Tsit5; grid_coarsening = true) 0.118 ± 0.0043 s 0.118 ± 0.0048 s 0.997
Simple Pendulum/IIP/Shooting(Tsit5()) 0.189 ± 0.0049 ms 0.189 ± 0.0053 ms 0.999
Simple Pendulum/OOP/BoundaryValueDiffEq.MIRK2() 0.0419 ± 0.002 s 0.0416 ± 0.00086 s 1.01
Simple Pendulum/OOP/BoundaryValueDiffEq.MIRK3() 12.2 ± 0.19 ms 11.9 ± 0.19 ms 1.02
Simple Pendulum/OOP/BoundaryValueDiffEq.MIRK4() 3.47 ± 0.088 ms 3.43 ± 0.08 ms 1.01
Simple Pendulum/OOP/BoundaryValueDiffEq.MIRK5() 3.55 ± 0.091 ms 3.5 ± 0.093 ms 1.01
Simple Pendulum/OOP/BoundaryValueDiffEq.MIRK6() 3.7 ± 0.1 ms 3.64 ± 0.11 ms 1.02
Simple Pendulum/OOP/MultipleShooting(10, Tsit5; grid_coarsening = false) 3.42 ± 0.94 ms 3.44 ± 0.92 ms 0.995
Simple Pendulum/OOP/MultipleShooting(10, Tsit5; grid_coarsening = true) 6.09 ± 1.3 ms 6.13 ± 1.4 ms 0.993
Simple Pendulum/OOP/MultipleShooting(100, Tsit5; grid_coarsening = false) 0.13 ± 0.013 s 0.132 ± 0.013 s 0.989
Simple Pendulum/OOP/MultipleShooting(100, Tsit5; grid_coarsening = true) 0.399 ± 0.0068 s 0.4 ± 0.0068 s 0.998
Simple Pendulum/OOP/Shooting(Tsit5()) 0.754 ± 0.03 ms 0.765 ± 0.039 ms 0.986
time_to_load 5.96 ± 0.032 s 5.94 ± 0.019 s 1

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@ChrisRackauckas
Copy link
Member

@ErikQQY is it sometimes an array sometimes an AbstractVectorOfArray? We should probably always make it an AbstractVectorofArray

@ErikQQY
Copy link
Member

ErikQQY commented Aug 3, 2024

@ErikQQY is it sometimes an array sometimes an AbstractVectorOfArray? We should probably always make it an AbstractVectorofArray

We always use Vector{Vector}} type in the nonlinear problems constructing and solving, should we change all of them to the AbstractVectorOfArray type instead? That could make the recursive flatten and unflatten of a Vector{Vector} much easier.

@ChrisRackauckas
Copy link
Member

Yes, Make it an AbstractDiffEqArray, since there's time associated with it as well.

@ChrisRackauckas
Copy link
Member

@ErikQQY can you help with the completion of this?

@ErikQQY
Copy link
Member

ErikQQY commented Aug 8, 2024

Sure, I will help to get this complete

@ErikQQY
Copy link
Member

ErikQQY commented Aug 9, 2024

The errors in this PR are mostly caused by the deprecation of indexing methods in RecursiveArrayTools.jl:

There are two ways of specifying boundary conditions when constructing a BVProblem, e.g. bc(u,p,t)=[u[1][1]] or bc(u,p,t)=[u(10.0)[1]] before RecursiveArrayTools.jl deprecate u[1] for u.u[1] or u[:,1] kind indexing, both of them were working fine, but now the centralized CI errors are mostly caused by the deprecation of this API.

So here is the solution:

We need to change the convention of defining boundary conditions as bc(u,p,t)=[u.u[1][1]] or bc(u,p,t)=[u[:,1][1]] now for this PR to continue. This may involve lots of tests, examples, and docs change.

@ChrisRackauckas
Copy link
Member

We need to change the convention of defining boundary conditions as bc(u,p,t)=[u.u[1][1]] or bc(u,p,t)=[u[:,1][1]] now for this PR to continue. This may involve lots of tests, examples, and docs change.

Yes, that's what needs to happen everywhere.

ChrisRackauckas and others added 5 commits August 16, 2024 06:30
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@ErikQQY ErikQQY closed this Aug 16, 2024
@ErikQQY ErikQQY reopened this Aug 16, 2024
ErikQQY and others added 2 commits August 17, 2024 16:49
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
ChrisRackauckas and others added 5 commits August 17, 2024 05:50
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@ChrisRackauckas
Copy link
Member

@avik-pal do you know why this ambiguity is still reported? I don't see it actually being hit?

@avik-pal
Copy link
Member

@ErikQQY
Copy link
Member

ErikQQY commented Aug 17, 2024

The Windows CI test looks weird, all other tests are working fine

@ChrisRackauckas ChrisRackauckas merged commit 4691581 into master Aug 17, 2024
5 of 7 checks passed
@ChrisRackauckas ChrisRackauckas deleted the at/use-reusable-workflows branch August 17, 2024 17:31
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.

4 participants