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

Fixed bug in sphConvolution.m #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

BenTM92
Copy link

@BenTM92 BenTM92 commented Jan 10, 2022

Fixed wrong indexing in sphConcolution.m

Fixed wrong indexing in sphConcolution.m
@polarch
Copy link
Owner

polarch commented Jan 10, 2022

Hi BenTM92,

I'm pretty sure the indexing is correct in this one, I have used it in the past quite a lot (?).

@polarch
Copy link
Owner

polarch commented Jan 10, 2022

Ah, I see, you assume that the filter kernel coefficients are in the full format, of (N+1)^2 of them, in which case your indexing is correct. However, since this is for axisymmetric kernel convolution (the simple case), the kernel is described fully by (N+1) coefficients, which is what I assumed it is passed here.

Both cases can be included though, maybe by passing an extra argument of {0,1} for 'compact' , 'full' ?

@BenTM92
Copy link
Author

BenTM92 commented Jan 10, 2022

Hi polarch,

oh that was my fault. I did not read the comments carefully. I've just used it with the reults of the leastSquaresSHT.m function, which results the full format.
This would be a great idea. So there is no confusion, if someonelse does something similar.

@BenTM92 BenTM92 closed this Jan 10, 2022
@BenTM92 BenTM92 reopened this Jan 10, 2022
@polarch
Copy link
Owner

polarch commented Jan 11, 2022

Ok, thanks. I'll add the option the soonest I can.

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