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: remove uses of issymbollike, depend on SII #346

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

AayushSabharwal
Copy link
Member

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (172cdc4) 84.99% compared to head (823a3b2) 82.69%.

Files Patch % Lines
src/interface/solution/common.jl 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #346      +/-   ##
==========================================
- Coverage   84.99%   82.69%   -2.30%     
==========================================
  Files          41       41              
  Lines        1959     1959              
==========================================
- Hits         1665     1620      -45     
- Misses        294      339      +45     

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

@xtalax
Copy link
Member

xtalax commented Dec 20, 2023

Solutions are coming out different, not sure why

@xtalax
Copy link
Member

xtalax commented Dec 21, 2023

I will fix tomorrow

@cmhyett
Copy link
Contributor

cmhyett commented Jan 16, 2024

I've been digging into this, it seems like the PDESolution interface is fine, but the tests compare against a sol.original_sol and I believe the interpolation error actually exists here.

To show there isn't actually a difference in the interior solution, see the plots:
SII_PR_justification_1
SII_PR_justification_end

And the numerical difference:

julia> sol[u(t,x,y)][1,:,:] .- traditional_sol[1,:,:]
9×9 Matrix{Float64}:
 -0.0438214  -0.0438214  -0.0438214  -0.0438214  -0.0438214  -0.0438214  -0.0438214  -0.0438214  -0.0438214
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
 -0.0438214   0.0         0.0         0.0         0.0         0.0         0.0         0.0         0.0
julia> sol[u(t,x,y)][end,:,:] .- traditional_sol[end,:,:]
9×9 Matrix{Float64}:
  0.013751    -0.00170683  0.0141994  0.0140295  0.0135991  0.01192  0.0154102  0.0238034  -0.00906672
 -0.00039773   0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0
  0.0503736    0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0
  0.395711     0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0
  1.56715      0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0
 -0.312651     0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0
 -0.217997     0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0
 -0.0124258    0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0
 -0.00906672   0.0         0.0        0.0        0.0        0.0      0.0        0.0         0.0

If someone could point me to where this sol.original_sol is calculated, perhaps I could root-cause the issue.

@cmhyett
Copy link
Contributor

cmhyett commented Jan 16, 2024

Ah, so there are some dubious calls in the failing test MethodOfLines/test/components/solution_interface.jl:42

grid = get_discrete(pdesys, discretization) #(9,9)
traditional_sol = zeros(Float64, length(sol.t), 9, 9)
solu = sol.original_sol[grid[u(t, x, y)]]

but sol.original_sol is only (T, 8, 8), and we're indexing into it with a (9,9). It seems like it's just repeating the u(t,x,y)[2,2] value for the boundaries.

julia> traditional_sol[1,:,:]
9×9 Matrix{Float64}:
 0.0438214  0.0438214    0.0438214    0.0438214    …  0.0438214    0.0438214    0.0438214    0.0438214
 0.0438214  0.0438214    0.456629     0.99737         0.0438214    0.000881502  3.71685e-6   3.28505e-9
 0.0438214  0.456629     0.99737      0.456629        0.000881502  3.71685e-6   3.28505e-9   6.08589e-13
 0.0438214  0.99737      0.456629     0.0438214       3.71685e-6   3.28505e-9   6.08589e-13  2.36331e-17
 0.0438214  0.456629     0.0438214    0.000881502     3.28505e-9   6.08589e-13  2.36331e-17  1.92368e-22
 0.0438214  0.0438214    0.000881502  3.71685e-6   …  6.08589e-13  2.36331e-17  1.92368e-22  3.28215e-28
 0.0438214  0.000881502  3.71685e-6   3.28505e-9      2.36331e-17  1.92368e-22  3.28215e-28  1.17382e-34
 0.0438214  3.71685e-6   3.28505e-9   6.08589e-13     1.92368e-22  3.28215e-28  1.17382e-34  8.79946e-42
 0.0438214  3.28505e-9   6.08589e-13  2.36331e-17     3.28215e-28  1.17382e-34  8.79946e-42  1.3827e-49

julia> traditional_sol[10,:,:]
9×9 Matrix{Float64}:
 0.0181737  0.0181737    0.0181737     0.0181737     0.0181737    …   0.0181737     0.0181737     0.0181737
 0.0181737  0.0181737   11.5379        5.80246       0.00283022       0.00158061    0.00607835    0.0145106
 0.0181737  4.65155      5.93072      11.4036        0.0053046        0.00264009    0.00842839    0.0189501
 0.0181737  2.48606      1.9606        0.0438791     0.000554888      0.000628334   0.000111981  -0.00974379
 0.0181737  1.02964      0.0456341     0.000454459   4.57115e-5       0.000588307   0.000738681   0.00106035
 0.0181737  0.90901      0.000600395   6.45301e-5   -1.19882e-5   …   0.000354857   0.000559608   0.000500321
 0.0181737  0.00121619   9.50736e-5   -3.92466e-5   -3.62156e-5      -0.000212254  -0.000265876  -6.59349e-5
 0.0181737  0.00549957  -1.42342e-5   -2.0833e-5     2.98726e-5       0.000196979   0.000532795   0.00116151
 0.0181737  0.00730426  -3.36528e-5   -1.85244e-6    1.52625e-5       0.000254279   0.00110834    0.00309887

julia> traditional_sol[end,:,:]
9×9 Matrix{Float64}:
 -0.013751  -0.013751   -0.013751     -0.013751     …  -0.013751     -0.013751    -0.013751    -0.013751
 -0.013751  -0.013751    0.0603288    -0.000233445     -0.000608095   0.00147142   0.00508782  -0.0141488
 -0.013751  16.9011     39.415         0.320138         0.00281172    0.00338428  -0.00508294   0.0366225
 -0.013751  17.3233     13.0935        0.0744963        0.0198935     0.0197856   -0.0830545    0.38196
 -0.013751  -0.453438    0.0423686    -0.00234209       0.0201236     0.0255276   -0.976563     1.55339
 -0.013751   8.80398     0.000831992   0.00336582   …   0.0158783     0.0273809    0.108254    -0.326402
 -0.013751  -0.596338    0.00110156    0.00313817      -0.0206732    -0.0179058    0.022136    -0.231748
 -0.013751  -0.0123999   0.000911816   0.00137948      -0.0028375    -0.00512977   0.0182841   -0.0261768
 -0.013751  -0.0154579   0.000448413   0.000278492     -0.00183106    0.0016592    0.0100524   -0.0228178

@cmhyett
Copy link
Contributor

cmhyett commented Jan 16, 2024

I think this is low risk acceptance that will clear bugs off the backlog for the issymbolic bit. I'd be happy to remedy the tests in another PR if @xtalax agrees with the above assessment.

@ChrisRackauckas
Copy link
Member

Okay, because https://github.com/SciML/MethodOfLines.jl/actions/runs/7283474229/job/19847405484?pr=346#step:6:533 scares me a bit, but you're saying it's not the solution interface but the interpolation that's the issue?

@cmhyett
Copy link
Contributor

cmhyett commented Jan 18, 2024

Yes.

As far as I can tell, this sol.original_sol is the raw output of the ODE solve, but because MoL replaces the boundary values (in this case for periodic BCs), it does not contain boundary information. But as we index into the ODESolution with an oversized grid, (and this is where I get a bit hazy and I'm less confident on what the test is trying to accomplish) I think some backend for ODESolution is trying to perform an interpolation, but it doesn't have the PDE information, so it just returns constants for the boundary values.

As the rest of the tests for the solution interface, and other tests that use the solution interface pass, I'm pretty confident the issue is with the test, rather than the solution interface.

@ChrisRackauckas ChrisRackauckas merged commit d0ba366 into SciML:master Jan 19, 2024
38 of 50 checks passed
@cmhyett
Copy link
Contributor

cmhyett commented Jan 19, 2024

Spinning up a PR to get the CI back in shape.

@ChrisRackauckas
Copy link
Member

thanks that will be immensely helpful

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