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

Eliminate redundant transformations when drawing WCSAxes ticks/gridlines #16362

Open
ayshih opened this issue Apr 30, 2024 · 4 comments · May be fixed by #16366
Open

Eliminate redundant transformations when drawing WCSAxes ticks/gridlines #16362

ayshih opened this issue Apr 30, 2024 · 4 comments · May be fixed by #16366

Comments

@ayshih
Copy link
Contributor

ayshih commented Apr 30, 2024

What is the problem this feature will solve?

When WCSAxes draws ticks/gridlines, it performs a pixel->world transform of (by default) a 50x50 grid of pixels, but it currently performs the identical calculation a total of four times: once when drawing ticks, again when drawing gridlines, and doubled because there are two components. These redundant calculations are particularly expensive when working with grid overlays (world->world) because then the coordinates framework is being used.

Describe the desired outcome

Caching should be implemented so that the pixel->world transform is recomputed only when necessary.

Additional context

Both CoordinateHelper._update_ticks() and CoordinateHelper._update_grid_lines() call CoordinatesMap.get_coord_range(), which in turn calls astropy.visualization.wcsaxes.coordinate_range.find_coordinate_range(). Unfortunately, there is no caching of the output of find_coordinate_range().

@astrofrog
Copy link
Member

Thanks for the diagnosis! I think the main thing to figure out is how to determine if the transform has changed compared to a previous call, i.e. how to determine the key for the hash. We might also want to see if it would be easier to do the caching at the Transform-class level since it's easier for transforms to be aware of whether any of their internal parameters have changed.

@astrofrog
Copy link
Member

One of the main difficulties with caching is going to be knowing if the WCS has changed in any way. For FITS WCS, hash(wcs) does not change even if some of the transformation parameters change.

If we can't figure this out, then the next best option will be to have some way in WCSAxes of temporarily setting some kind of context within which the four transformation will happen and assuming that within that context the WCS won't get modified (which is reasonable). We might need to do this because we will probably need to assume within this that the coordinate transformation graph doesn't get modified.

@ayshih
Copy link
Contributor Author

ayshih commented May 1, 2024

If we can't figure this out, then the next best option will be to have some way in WCSAxes of temporarily setting some kind of context within which the four transformation will happen

Ooh, that's a simple solution. The four transformations are actually all tightly grouped in the following loop (._draw_grid() calls both ._update_ticks() and ._update_grid_lines()), so a context isn't even really necessary:

for coord in coords:
coord._draw_grid(renderer)

It should be straightforward to compute the coord range once before this loop and then have everything within utilize the same output. Draft PR incoming!

@ayshih
Copy link
Contributor Author

ayshih commented May 1, 2024

#16366 solves the immediate issue by reducing the four transformations to just one. It doesn't do anything for redundant transformations if WCSAxes.draw_grid() is called multiple times with an unchanged WCS, but that's beyond the scope of what I meant by this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants