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

Allow setting spherical screen radius #7532

Merged
merged 15 commits into from
Jul 11, 2024
Merged

Conversation

svank
Copy link
Contributor

@svank svank commented Mar 26, 2024

With Helioprojective frames, you can use assume_spherical_screen to transform coordinates into another frame. The screen's center is configurable, but the radius is always such that the screen passes through the Sun. That's usually the right choice, but a user (i.e. me) might want to change the radius of the screen (with it then not passing through the Sun). This PR allows that. Delightfully, the spherical screen part of Helioprojective.make_3d is written in full generality, so all that's needed is an argument to assume_spherical_screen allowing a radius to be passed in.

It looks like there weren't any tests for assume_spherical_screen (though there was coverage through examples in the docs), so I added a one and exercise this option.

My use case here is making videos with WISPR on PSP, which involves compositing two images from the two imagers, which are taken at slightly different times and locations. It's a wide FOV that doesn't include the Sun, but the FOV is ~fixed in helioprojective coordinates. A spherical screen passing through the Sun starts to be uncomfortably close to PSP (perihelia near 10 R_sun, while traveling ~10 R_sun/day). Projecting the images onto that kinda-close screen to composite them adds a bit of jittery parallax for the stars, which makes videos look a bit jumpy. As an experiment, I'm instead projecting onto a screen at infinity, which has the stars move very smoothly and evenly through the FOV---in trade, the coronal structures should have a bit of parallax, but since they're large, diffuse, and evolving, it's less noticeable. (My only goal in this is pretty videos.)

@svank svank requested a review from a team as a code owner March 26, 2024 15:55
@nabobalis nabobalis added coordinates Affects the coordinates submodule Needs Review Needs reviews before merge No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) labels Mar 26, 2024
@ayshih
Copy link
Member

ayshih commented Mar 27, 2024

@svank: It sounds like you're setting the radius to something very large. If I actually finish #7115 to make a planar screen available, would that satisfy your use case?

@svank
Copy link
Contributor Author

svank commented Mar 27, 2024

I think the planar screen wouldn't help, since it also (as implemented there) passes through the Sun, and my WISPR FOV goes from ~10 to 110 degrees of helioprojective longitude. So either the plane would be far away in the near-the-Sun side of the FOV but never actually touch the lines of sight on the far-from-the-Sun side, or it would be far away on the far-from-the-Sun side but much too close on the near-the-Sun side.

Though that got me thinking more, and I can get a screen at infinity without this PR by placing the "observer" location (the sphere center) lightyears away and in the right position so that the sphere passes through the Sun, right behind PSP, and then out to infinity:

     ___________      ___
    /           \      |
   /             \     m
  /               \    a
 /                 \   n
|                   |  y
|         ."observer"  
|                   |  l
|                   |  y
 \  \ FOV   ⁄      /   |
  \  \    ⁄       /    |
   \  \ ⁄        /     |
Sun *__+________/     ---
       PSP           

So I can fudge it without this PR, though it's not quite as clean as just having a really big sphere centered on my actual observer.

@nabobalis nabobalis added this to the 6.0.0 milestone May 15, 2024
@wtbarnes
Copy link
Member

wtbarnes commented May 21, 2024

Given how #7115 has now evolved, I think it would be preferable to make this change on that PR as an additional keyword argument to SphericalScreen. The test could be ported directly over and just modified to use SphericalScreen rather than assume_spherical_screen. Alternatively, we could merge that PR first and then alter this PR to accomodate the change in the API. Let's just merge #7115 first and then modify this PR to accommodate that change.

Sorry this has been tied up for so long @svank. We would really like to have this in v6.0.

@nabobalis nabobalis removed the Needs Review Needs reviews before merge label Jul 3, 2024
@wtbarnes wtbarnes requested review from a team as code owners July 4, 2024 18:25
@nabobalis nabobalis added the Needs Review Needs reviews before merge label Jul 4, 2024
@nabobalis
Copy link
Contributor

Doc issues:

/home/runner/work/sunpy/sunpy/.tox/build_docs/lib/python3.12/site-packages/sunpy/coordinates/screens.py:docstring of sunpy.coordinates.screens.SphericalScreen:1: WARNING: py:class reference target not found: Unit('m')
/home/runner/work/sunpy/sunpy/.tox/build_docs/lib/python3.12/site-packages/sunpy/coordinates/screens.py:docstring of sunpy.coordinates.screens.SphericalScreen:1: WARNING: py:class reference target not found: Unit('m')
<unknown>:97: WARNING: py:obj reference target not found: sunpy.coordinates.Helioprojective.SphericalScreen

@wtbarnes
Copy link
Member

wtbarnes commented Jul 4, 2024

Doc issues:

/home/runner/work/sunpy/sunpy/.tox/build_docs/lib/python3.12/site-packages/sunpy/coordinates/screens.py:docstring of sunpy.coordinates.screens.SphericalScreen:1: WARNING: py:class reference target not found: Unit('m')
/home/runner/work/sunpy/sunpy/.tox/build_docs/lib/python3.12/site-packages/sunpy/coordinates/screens.py:docstring of sunpy.coordinates.screens.SphericalScreen:1: WARNING: py:class reference target not found: Unit('m')
<unknown>:97: WARNING: py:obj reference target not found: sunpy.coordinates.Helioprojective.SphericalScreen

I fixed the last one, but am really confused by the first two. I assume this is coming from the unit annotation in the function signature, but I don't know why Sphinx wouldn't be able to find that target, especially since it seems to have no issue with it in PlanarScreen

@nabobalis
Copy link
Contributor

Doc issues:

/home/runner/work/sunpy/sunpy/.tox/build_docs/lib/python3.12/site-packages/sunpy/coordinates/screens.py:docstring of sunpy.coordinates.screens.SphericalScreen:1: WARNING: py:class reference target not found: Unit('m')
/home/runner/work/sunpy/sunpy/.tox/build_docs/lib/python3.12/site-packages/sunpy/coordinates/screens.py:docstring of sunpy.coordinates.screens.SphericalScreen:1: WARNING: py:class reference target not found: Unit('m')
<unknown>:97: WARNING: py:obj reference target not found: sunpy.coordinates.Helioprojective.SphericalScreen

I fixed the last one, but am really confused by the first two. I assume this is coming from the unit annotation in the function signature, but I don't know why Sphinx wouldn't be able to find that target, especially since it seems to have no issue with it in PlanarScreen

I think it's best to add it the nitpicky file. We do this for a bunch of other units.

@nabobalis nabobalis requested review from ayshih and removed request for a team July 4, 2024 22:57
@wtbarnes
Copy link
Member

wtbarnes commented Jul 5, 2024

@svank Could you have a quick look over this to make ensure it preserves the original intent of your PR?

@svank
Copy link
Contributor Author

svank commented Jul 8, 2024

Yeah, a quick test shows I can do what I originally wanted with this PR. Thanks for pushing this all along!

@nabobalis nabobalis removed the request for review from ayshih July 8, 2024 19:07
Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

My only comment is to make the radius keyword keyword-only (i.e., stick a star before it)

wtbarnes and others added 2 commits July 8, 2024 16:34
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
@nabobalis nabobalis requested a review from ayshih July 8, 2024 21:02
Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

Also add "See Also" sections to each screen docstring that points at the other screen

sunpy/coordinates/screens.py Outdated Show resolved Hide resolved
Co-authored-by: Albert Y. Shih <ayshih@gmail.com>
@nabobalis nabobalis removed the Needs Review Needs reviews before merge label Jul 9, 2024
@wtbarnes wtbarnes merged commit 2d99694 into sunpy:main Jul 11, 2024
27 of 28 checks passed
@nabobalis
Copy link
Contributor

Thank you again @svank!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coordinates Affects the coordinates submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants