Skip to content

Conversation

@aurelio-amerio
Copy link

Hello. Following https://healpix.jpl.nasa.gov/html/Healpix_cxx/healpix__base2_8cc-source.html I have implemented the function getAllNeighbours to find the pixels around a given pixel in ring/nest ordering. I have also added a test for it.

You might want to consider whether to rename it "getAllNeighbors", to comform the name with American English...

I hope the contribution is useful!

@ziotom78
Copy link
Collaborator

Thanks a lot! I've added a few review comments. May you please add some documentation for the function in the manual as well? (Adding getAllNeighboursRing and getAllNeighboursNest to the list at the end of file docs/src/pixelfunc.md would be enough.)

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

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

Comparison is base (640832f) 91.08% compared to head (38b2701) 89.29%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
- Coverage   91.08%   89.29%   -1.79%     
==========================================
  Files          20       20              
  Lines        1447     1495      +48     
==========================================
+ Hits         1318     1335      +17     
- Misses        129      160      +31     
Files Coverage Δ
src/pixelfunc.jl 87.80% <32.55%> (-9.74%) ⬇️

... and 2 files with indirect coverage changes

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

@aurelio-amerio
Copy link
Author

Hello, I'm not sure where to see your review comments, I only see the codecov comments and will work on them as soon as possible. I'll also add the documentation bit, and a function with Ring/Nest in the name. Anything else?

@ziotom78
Copy link
Collaborator

Hello, I'm not sure where to see your review comments, I only see the codecov comments and will work on them as soon as possible. I'll also add the documentation bit, and a function with Ring/Nest in the name. Anything else?

Thanks a lot! The review comments should be displayed above in this very page (I can see them right now), they appear as normal posts but are associated with specific code snippets.

@aurelio-amerio
Copy link
Author

I just wanted to say I've been very busy with work lately and I did't get to do the necessary updates. I will do it as soon as I have a bit more free time

@ziotom78
Copy link
Collaborator

I just wanted to say I've been very busy with work lately and I did't get to do the necessary updates. I will do it as soon as I have a bit more free time

Don't worry! We're all very busy.

@abhro abhro closed this Dec 23, 2025
@abhro abhro reopened this Dec 23, 2025
@abhro
Copy link
Member

abhro commented Dec 23, 2025

Closed and re-opened to trigger checks

@codecov
Copy link

codecov bot commented Dec 23, 2025

Codecov Report

❌ Patch coverage is 32.55814% with 29 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@640832f). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/pixelfunc.jl 32.55% 29 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #107   +/-   ##
=========================================
  Coverage          ?   89.20%           
=========================================
  Files             ?       20           
  Lines             ?     1473           
  Branches          ?        0           
=========================================
  Hits              ?     1314           
  Misses            ?      159           
  Partials          ?        0           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +651 to +672
# auxiliary quantites needed to obtain neughbouring pixels, implementation following https://healpix.jpl.nasa.gov/html/Healpix_cxx/healpix__base2_8cc-source.html
_xoffset = @SVector [-1, -1, 0, 1, 1, 1, 0, -1]
_yoffset = @SVector [0, 1, 1, 1, 0, -1, -1, -1]
_facearray = @SMatrix [[8, 9, 10, 11, -1, -1, -1, -1, 10, 11, 8, 9];; # S
[5, 6, 7, 4, 8, 9, 10, 11, 9, 10, 11, 8];; # SE
[-1, -1, -1, -1, 5, 6, 7, 4, -1, -1, -1, -1];; # E
[4, 5, 6, 7, 11, 8, 9, 10, 11, 8, 9, 10];; # SW
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11];; # center
[1, 2, 3, 0, 0, 1, 2, 3, 5, 6, 7, 4];; # NE
[-1, -1, -1, -1, 7, 4, 5, 6, -1, -1, -1, -1];; # W
[3, 0, 1, 2, 3, 0, 1, 2, 4, 5, 6, 7];; # NW
[2, 3, 0, 1, -1, -1, -1, -1, 0, 1, 2, 3]] # N

_swaparray = @SMatrix [[0, 0, 0, 0, 0, 0, 0, 0, 3, 3, 3, 3];; # S
[0, 0, 0, 0, 0, 0, 0, 0, 6, 6, 6, 6];; # SE
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0];; # E
[0, 0, 0, 0, 0, 0, 0, 0, 5, 5, 5, 5];; # SW
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0];; # center
[5, 5, 5, 5, 0, 0, 0, 0, 0, 0, 0, 0];; # NE
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0];; # W
[6, 6, 6, 6, 0, 0, 0, 0, 0, 0, 0, 0];; # NW
[3, 3, 3, 3, 0, 0, 0, 0, 0, 0, 0, 0]] # N
Copy link
Member

Choose a reason for hiding this comment

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

Should these be marked const just for a bit more security?

Comment on lines +693 to +699
for m in 1:8
if nest
result[m] = xyf2pixNest(resol, ix + _xoffset[m], iy + _yoffset[m], face_num)
else
result[m] = xyf2pixRing(resol, ix + _xoffset[m], iy + _yoffset[m], face_num)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

We can probably select the function outside of the loop since nest shouldn't change inside

Suggested change
for m in 1:8
if nest
result[m] = xyf2pixNest(resol, ix + _xoffset[m], iy + _yoffset[m], face_num)
else
result[m] = xyf2pixRing(resol, ix + _xoffset[m], iy + _yoffset[m], face_num)
end
end
fn = nest ? xyf2pixNest : xyf2pixRing
for m in 1:8
result[m] = fn(resol, ix + _xoffset[m], iy + _yoffset[m], face_num)
end

Comment on lines +701 to +738
for i in 1:8
x = ix + _xoffset[i]
y = iy + _yoffset[i]
nbnum = 4
if x < 0
x += nside_
nbnum -= 1
elseif x >= nside_
x -= nside_
nbnum += 1
end
if y < 0
y += nside_
nbnum -= 3
elseif y >= nside_
y -= nside_
nbnum += 3
end

f = _facearray[face_num+1, nbnum+1]
if f >= 0
if _swaparray[face_num+1, nbnum+1] & 1 != 0
x = nside_ - x - 1
end
if _swaparray[face_num+1, nbnum+1] & 2 != 0
y = nside_ - y - 1
end
if _swaparray[face_num+1, nbnum+1] & 4 != 0
x, y = y, x
end
if nest
result[i] = xyf2pixNest(resol, x, y, f)
else
result[i] = xyf2pixRing(resol, x, y, f)
end
else
result[i] = -1
end
Copy link
Member

Choose a reason for hiding this comment

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

Same here, about choosing the function outside the loop

Suggested change
for i in 1:8
x = ix + _xoffset[i]
y = iy + _yoffset[i]
nbnum = 4
if x < 0
x += nside_
nbnum -= 1
elseif x >= nside_
x -= nside_
nbnum += 1
end
if y < 0
y += nside_
nbnum -= 3
elseif y >= nside_
y -= nside_
nbnum += 3
end
f = _facearray[face_num+1, nbnum+1]
if f >= 0
if _swaparray[face_num+1, nbnum+1] & 1 != 0
x = nside_ - x - 1
end
if _swaparray[face_num+1, nbnum+1] & 2 != 0
y = nside_ - y - 1
end
if _swaparray[face_num+1, nbnum+1] & 4 != 0
x, y = y, x
end
if nest
result[i] = xyf2pixNest(resol, x, y, f)
else
result[i] = xyf2pixRing(resol, x, y, f)
end
else
result[i] = -1
end
fn = nest ? xyf2pixNest : xyf2pixRing
for i in 1:8
x = ix + _xoffset[i]
y = iy + _yoffset[i]
nbnum = 4
if x < 0
x += nside_
nbnum -= 1
elseif x >= nside_
x -= nside_
nbnum += 1
end
if y < 0
y += nside_
nbnum -= 3
elseif y >= nside_
y -= nside_
nbnum += 3
end
f = _facearray[face_num+1, nbnum+1]
if f >= 0
if _swaparray[face_num+1, nbnum+1] & 1 != 0
x = nside_ - x - 1
end
if _swaparray[face_num+1, nbnum+1] & 2 != 0
y = nside_ - y - 1
end
if _swaparray[face_num+1, nbnum+1] & 4 != 0
x, y = y, x
end
result[i] = fn(resol, x, y, f)
else
result[i] = -1
end

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.

3 participants