Skip to content

Visibility recentering for wgridder - #53

Open
mreineck wants to merge 4 commits into
ducc0from
recentering
Open

Visibility recentering for wgridder#53
mreineck wants to merge 4 commits into
ducc0from
recentering

Conversation

@mreineck

Copy link
Copy Markdown
Owner

No description provided.

Comment thread src/ducc0/wgridder/wgridder_impl.h Outdated
double nm1 = (tmp>=0) ? (-xsq-ysq)/(sqrt(tmp)+1) : -sqrt(-tmp)-1;
double phs = w*(nm1+nshift);
// re-centering
phs += bl.r_l*x + bl.r_m*y;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The shift applies to n, not n*w (it is especially 0 at w=0!).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
phs += bl.r_l*x + bl.r_m*y;
nm1 += bl.r_l*x + bl.r_m*y;
double phs = w*(nm1+nshift);

if (tmp>=0)
{
// accurate form of sqrt(1-xsq-ysq)-1 for nm1 close to zero
auto nm1 = (-xsq-ysq)/(sqrt(tmp)+1);

@scpmw scpmw Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You will need to apply the transformation with grid correction as well

Suggested change
nm1 += bl.r_l*(x0+i*pixsize_x) + bl.r_m*(y0+j*pixsize_y);

if (divide_by_n)
fct /= nm1+1;
}
else // beyond the horizon, don't really know what to do here

@scpmw scpmw Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

NB - this actually makes figuring out what to do beyond the horizon harder, at least for my code. The reason is that I'm only checking the four corners, and when "extending" n like this beyond the horizon (which I think is generally a good idea to prevent "jumps"), there's situations where the re-centering "overpowers" the extension so that the maximum or minimum happens right at the horizon line instead of the image corners, causing the grid correction function to go out of its useful range at those points, tanking accuracy.

Not entirely sure whether that would happen here, just be warned that this is one of the small gotchas of the optimisation.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Thank you very much for having a look at this! I'll try to apply the changes as soon as I get a chance.
Yes, I can imagine that this will make the "over-the-horizon" mode even nastier, and I'm really glad to have got that working somehow in the current version. If I get the recentering to work in the more normal scenarios, I'll already be very happy, and whenever I detect n dropping below 0, I'll just disable the optimization until there is an implementation I'm really confident in.

@scpmw scpmw Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just to be clear - I think you are relatively immune to the particular problem I ran into because you check every pixel in the image when determining nm1min/nm1max. Arguably I am just getting bitten by my own attempt to micro-optimise this by only checking the corners. It is just really tempting because once you choose r_l and r_m as proposed you always end up with the maximum in the facet centre and the minima in the corners... except when the horizon is in the picture, which is a bit unsatisfying.

@mreineck

Copy link
Copy Markdown
Owner Author

I applied your suggestions, but the unit tests are still unhappy when r_l or r_m become nonzero.
It seems that the n' coordinate can actually become larger than 1 ... perhaps this breaks some kind of implicit assumption I made so far. Will continue investigating.

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