Skip to content

Simplify Zernike Jacobi matrices + bugfixes #123

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

Merged
merged 12 commits into from
May 4, 2022

Conversation

TSGut
Copy link
Member

@TSGut TSGut commented Apr 27, 2022

This makes the fixes made in JuliaLinearAlgebra/InfiniteLinearAlgebra.jl#103 also work for the Zernike Jacobi matrices.

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #123 (2b3012d) into master (83421e6) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   96.23%   96.15%   -0.09%     
==========================================
  Files           4        4              
  Lines         718      702      -16     
==========================================
- Hits          691      675      -16     
  Misses         27       27              
Impacted Files Coverage Δ
src/disk.jl 98.08% <100.00%> (-0.14%) ⬇️

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 83421e6...2b3012d. Read the comment docs.

@TSGut TSGut changed the title WIP: Bugfix: Addition / Multiplication of Zernike Jacobi matrices Bugfix: Addition / Multiplication of Zernike Jacobi matrices Apr 27, 2022
@TSGut
Copy link
Member Author

TSGut commented Apr 27, 2022

I think this covers all the actions one may want to do with these operators for now. Ready to merge.

@TSGut
Copy link
Member Author

TSGut commented Apr 27, 2022

@dlfivefifty So what's the mechanism allowing for the use of Z \ (x .* Z) to call the jacobi matrices? That is still missing to make this implementation harmonize properly with the rest of the package. Tried to see what happens for the Triangle and Xu Disk by debug entering but the calls seemed more complicated than I remember it from e.g. the fractional Laplacian implementation I did.

@dlfivefifty
Copy link
Member

@TSGut TSGut changed the title Bugfix: Addition / Multiplication of Zernike Jacobi matrices Add and multiply Zernike Jacobi matrices + copy bugfix Apr 29, 2022
@TSGut
Copy link
Member Author

TSGut commented Apr 29, 2022

I figured out why it wasn't working. Basically, the stuff you linked at one point requires copy(jacobimatrix(Val(1),Z)) to work but that wasn't working because copy wasn't defined for the type I made to hide the excessive type printout so it defaulted to something for arbitrary block arrays which doesn't work on the infinite case.

I think I fixed it by overloading copy for the bands type.

@TSGut
Copy link
Member Author

TSGut commented Apr 29, 2022

@dlfivefifty This is ready to merge I think.

@TSGut TSGut changed the title Add and multiply Zernike Jacobi matrices + copy bugfix Simplify Zernike Jacobi matrices + bugfixes May 3, 2022
@TSGut
Copy link
Member Author

TSGut commented May 3, 2022

By comparing with the Triangle code, I've decided it's better to not have the custom type for the Jacobi matrices for now, especially since it just leads to harder to read and debug code and actually slows things down for an enduser.

I've also removed the BlockHcat call. As a result, the tests I implemented here won't pass until the addition bug mentioned here #123 (comment) has been resolved.

@TSGut
Copy link
Member Author

TSGut commented May 4, 2022

@dlfivefifty This is ready to merge and can now do polynomial multiplication operators. I'm pretty sure the coverage reduction is an artifact due to a reduction in lines of code as the patch diff is 100%.

@dlfivefifty dlfivefifty merged commit d36576f into JuliaApproximation:master May 4, 2022
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