Skip to content

Conversation

@oscardssmith
Copy link
Member

This also makes the u and du fields into regular Array instead of a NVector. This yak had a lot of hair.

oscarddssmith added 2 commits May 23, 2023 12:48
… /48	./test/common_interface

124	./test
140	./src/common_interface
180	./src
28	./gen
24	./.github/workflows
32	./.github
388	./lib
8	./.git/objects/35
8	./.git/objects/d0
8	./.git/objects/c0
8	./.git/objects/43
12	./.git/objects/42
8	./.git/objects/72
12	./.git/objects/48
8	./.git/objects/a4
8	./.git/objects/cb
8	./.git/objects/f4
8	./.git/objects/8f
8	./.git/objects/b8
8	./.git/objects/33
12	./.git/objects/e1
20	./.git/objects/4c
8	./.git/objects/52
12	./.git/objects/62
8	./.git/objects/d4
16	./.git/objects/bf
8	./.git/objects/09
8	./.git/objects/2e
8	./.git/objects/9e
8	./.git/objects/af
8	./.git/objects/6a
8	./.git/objects/53
8	./.git/objects/0e
8	./.git/objects/d1
8	./.git/objects/27
8	./.git/objects/71
16	./.git/objects/f5
16	./.git/objects/a7
8	./.git/objects/06
8	./.git/objects/94
8	./.git/objects/a5
8	./.git/objects/ba
12	./.git/objects/c1
12	./.git/objects/80
8	./.git/objects/b4
8	./.git/objects/0a
8	./.git/objects/97
8	./.git/objects/08
8	./.git/objects/dd
8	./.git/objects/fa
12	./.git/objects/54
8	./.git/objects/e6
12	./.git/objects/f6
16	./.git/objects/4f
8	./.git/objects/9d
8	./.git/objects/39
8	./.git/objects/d7
8	./.git/objects/cf
12	./.git/objects/fe
8	./.git/objects/bb
8	./.git/objects/6d
8	./.git/objects/db
16	./.git/objects/12
8	./.git/objects/96
8	./.git/objects/81
8	./.git/objects/c6
8	./.git/objects/e9
12	./.git/objects/65
8	./.git/objects/74
8	./.git/objects/17
8	./.git/objects/a9
8	./.git/objects/b6
20	./.git/objects/88
8	./.git/objects/07
24	./.git/objects/78
1404	./.git/objects/pack
8	./.git/objects/fb
12	./.git/objects/98
12	./.git/objects/c2
24	./.git/objects/df
8	./.git/objects/37
8	./.git/objects/5a
16	./.git/objects/c5
16	./.git/objects/e7
8	./.git/objects/3c
8	./.git/objects/6e
8	./.git/objects/2f
8	./.git/objects/11
12	./.git/objects/22
8	./.git/objects/b3
8	./.git/objects/85
8	./.git/objects/ac
12	./.git/objects/69
8	./.git/objects/a0
8	./.git/objects/9b
8	./.git/objects/f1
12	./.git/objects/6b
20	./.git/objects/46
8	./.git/objects/9c
8	./.git/objects/24
12	./.git/objects/ab
8	./.git/objects/fd
8	./.git/objects/d6
8	./.git/objects/f3
8	./.git/objects/da
8	./.git/objects/a3
12	./.git/objects/1f
8	./.git/objects/16
8	./.git/objects/89
8	./.git/objects/d2
8	./.git/objects/38
8	./.git/objects/59
8	./.git/objects/77
8	./.git/objects/eb
24	./.git/objects/55
8	./.git/objects/70
20	./.git/objects/fc
12	./.git/objects/b5
8	./.git/objects/2a
8	./.git/objects/3b
8	./.git/objects/5b
12	./.git/objects/91
12	./.git/objects/92
8	./.git/objects/18
8	./.git/objects/2c
20	./.git/objects/51
8	./.git/objects/ae
4	./.git/objects/info
8	./.git/objects/66
8	./.git/objects/1d
12	./.git/objects/e0
8	./.git/objects/25
12	./.git/objects/61
2656	./.git/objects
24	./.git/logs/refs/heads
8	./.git/logs/refs/remotes/origin/compathelper/new_version
12	./.git/logs/refs/remotes/origin/compathelper
32	./.git/logs/refs/remotes/origin
8	./.git/logs/refs/remotes/upstream/compathelper/new_version
12	./.git/logs/refs/remotes/upstream/compathelper
32	./.git/logs/refs/remotes/upstream
68	./.git/logs/refs/remotes
100	./.git/logs/refs
112	./.git/logs
4	./.git/branches
24	./.git/refs/heads
16	./.git/refs/tags
8	./.git/refs/remotes/origin/compathelper/new_version
12	./.git/refs/remotes/origin/compathelper
32	./.git/refs/remotes/origin
8	./.git/refs/remotes/upstream/compathelper/new_version
12	./.git/refs/remotes/upstream/compathelper
32	./.git/refs/remotes/upstream
68	./.git/refs/remotes
116	./.git/refs
64	./.git/hooks
12	./.git/rr-cache/39f1328cef5f571b810bfa24b7568577532267b0
16	./.git/rr-cache
8	./.git/info
3028	./.git
3836	. be s rather than s.
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #404 (ae9bd72) into master (f4d7801) will increase coverage by 1.29%.
The diff coverage is 83.53%.

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
+ Coverage   80.02%   81.32%   +1.29%     
==========================================
  Files          11       11              
  Lines        1427     1435       +8     
==========================================
+ Hits         1142     1167      +25     
+ Misses        285      268      -17     
Impacted Files Coverage Δ
src/common_interface/integrator_utils.jl 84.86% <80.00%> (+1.88%) ⬆️
src/common_interface/solve.jl 82.51% <81.39%> (+0.58%) ⬆️
src/common_interface/integrator_types.jl 71.42% <88.88%> (-0.80%) ⬇️
src/common_interface/function_types.jl 94.04% <100.00%> (+0.29%) ⬆️

... and 5 files with indirect coverage changes

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

Comment on lines +191 to +192
u_nvec::NVector
du_nvec::NVector
Copy link
Member

Choose a reason for hiding this comment

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

Do these alias u and du?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

Maybe make a comment.

@ChrisRackauckas
Copy link
Member

Why is this change only to IDA? I would think that some of these things are changing a style choice that we would want to keep uniform to the other solvers?

@oscardssmith
Copy link
Member Author

oscardssmith commented May 23, 2023

I started by only changing IDA to make sure this seems like a reasonable approach. Should I change the rest of the integrators?

@ChrisRackauckas
Copy link
Member

Yes, I think it would be good to further enforce Array like this everywhere. Also CVODE tests non-vector things in much more detail so that would be a good test that the right things are vec and reshaped. I think by the aliasing of NVector to the original array it works out, but it would be good to be very sure.

@oscardssmith
Copy link
Member Author

I think this is ready, but if possible I would like to get this run on the MTK testsuite before merging (since I'm not 100% sure I haven't missed anything. This is a pretty big PR).

@ChrisRackauckas
Copy link
Member

This all looks reasonable. Add the MTK downstream from here? I don't know if MTK tests call Sundials though.

@ChrisRackauckas ChrisRackauckas merged commit 452d3bd into SciML:master May 23, 2023
@oscardssmith oscardssmith deleted the IDA-overhaul branch May 23, 2023 21:08
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