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

What to do about physics.solar_rotation #5965

Closed
wtbarnes opened this issue Mar 15, 2022 · 4 comments · Fixed by #6031
Closed

What to do about physics.solar_rotation #5965

wtbarnes opened this issue Mar 15, 2022 · 4 comments · Fixed by #6031
Labels
Discussion An issue opened for, or undergoing discussion. physics Affects the physics submodule

Comments

@wtbarnes
Copy link
Member

Provide a general description of the issue or problem.

When deprecating image.coalignment (#5957), it was revealed that physics.solar_rotation.mapsequence_solar_derotate depends on image.coalignment.apply_shifts. This led me down the rabbit hole of the physics.solar_rotation module.

physics.solar_rotation contains two functions:

  • calculate_solar_rotate_shift
  • mapsequence_solar_derotate

The latter is just a light wrapper around apply_shifts (which as mentioned above, we would like to deprecate). The former is just used by the latter to compute the shifts associated with rotation relative to some selected layer.

I've mainly created this issue to discuss what we should do with this module. Leave it alone? Deprecate it and move it to sunkit-image?

Given that it is such a lightweight wrapper, I could also imagine just adding an example gallery entry in sunkit-image on how to use solar_rotate_coordinate with apply_shifts to shift a map according to some relative shift in the center coordinate of the map. This would also make it explicitly to users what is happening under the hood, i.e. there is no warping or rotation, just shifts in the image plane.

@wtbarnes wtbarnes added physics Affects the physics submodule Discussion An issue opened for, or undergoing discussion. labels Mar 15, 2022
@dstansby
Copy link
Member

Reading the docs for these, I'm downright confused by them. How is it even valid/possible to shift a map observed in a helioprojective coordinate system to compensate for solar rotation (which is a (latitude dependent) shift in Carrington longitude)? I vote we deprecate these, and point users to an example to differentially de-rotate a full map by a certain time interval.

@ayshih
Copy link
Member

ayshih commented Mar 16, 2022

@dstansby: I think you're over-thinking it. The purpose of this module is apply shifts to a sequence of maps so that the centers of the maps are all pointed at the same spot on the solar surface, accounting for solar rotation. This simulates a telescope tracking, say, an active region over time. A user may not want to actually transform/reproject the maps at all.

@dstansby
Copy link
Member

Ah, that makes sense. I think I am confused by the multiple meanings rotate have within our API. I think at least updating the function names (with deprecation etc. etc.) here to remove rotate or rotation would be a positive step. This would avoid confusion with:

  • Rotating maps around the center of the Sun
  • Differentially rotating maps forward/backwards in time
    I tried to brainstorm some other name ideas, but am a bit stumped though...

@wtbarnes
Copy link
Member Author

My proposed solution:

  • Move both calculate_solar_rotate_shift and mapsequence_solar_derotate to sunkit_image.coalignment and rename mapsequence_solar_derotate to mapsequence_coalign_by_derotation (or something like that...it at least mirrors the naming convention of mapsequence_coalign_by_match_template which does the same thing, except calculates the shifts according to a cross-correlation to some layer in the sequence(
  • Deprecate both of these functions and point people towards sunkit image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion An issue opened for, or undergoing discussion. physics Affects the physics submodule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants