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

Deprecate image.coalignment #5957

Merged
merged 11 commits into from
Mar 17, 2022
Merged

Conversation

wtbarnes
Copy link
Member

@wtbarnes wtbarnes commented Mar 13, 2022

image.coalignment has been moved to sunkit-image (See sunpy/sunkit-image#78).
This PR deprecates the coalignment module.
This deprecation will be included in 4.0 and thus we can remove the module in 4.1

Addresses #5425.

PR Checklist

  • I have followed the guidelines in the Contributing document
  • Changes follow the coding style of this project
  • Changes have been formatted and linted
  • Changes pass pytest style unit tests (and pytest passes).
  • Changes include any required corresponding changes to the documentation
  • Changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • Changes have a descriptive commit message with a short title
  • I have added a Fixes #XXXX - or Closes #XXXX - comment to auto-close the issue that your PR addresses

@wtbarnes wtbarnes requested a review from a team as a code owner March 13, 2022 21:56
@wtbarnes wtbarnes added image Affects the image submodule 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 13, 2022
@wtbarnes
Copy link
Member Author

wtbarnes commented Mar 13, 2022

The doc fail is real. I've messed up the use of .. warning:: in the module docstring.

I take that back. It seems that sphinx does not like having the warning raised in coalignment.py.

@wtbarnes
Copy link
Member Author

I just ended up slapping the deprecated decorator on every function.

changelog/5957.deprecation.rst Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
sunpy/image/coalignment.py Outdated Show resolved Hide resolved
sunpy/image/coalignment.py Outdated Show resolved Hide resolved
@wtbarnes
Copy link
Member Author

Well this has become slightly more complicated because of the use of apply_shifts in mapsequence_solar_derotate which I didn't realize until now,

return apply_shifts(mc, yshift_keep, xshift_keep, clip=clip)

Three possible options:

  • Do not deprecate apply_shifts or calculate_clipping
  • Move apply_shifts and calculate_clipping to physics.solar_rotation
  • Deprecate mapsequence_solar_derotate as well.

@dstansby
Copy link
Member

I guess the pragmatic option is move the required code but make it private, and then we can make a decision on what to do about mapsequence_solar_derotate independently of this PR?

@wtbarnes
Copy link
Member Author

I guess the pragmatic option is move the required code but make it private, and then we can make a decision on what to do about mapsequence_solar_derotate independently of this PR?

That seems sensible. I had kind of forgotten that physics.solar_rotation existed...

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

One of the remote data test fails is real: https://github.com/sunpy/sunpy/runs/5557145861?check_suite_focus=true#step:7:5237 and I've just rebooted the figure tests to check them (I think the fail was the MPL issue that's now fixed)

@wtbarnes wtbarnes requested a review from a team as a code owner March 17, 2022 20:51
@wtbarnes
Copy link
Member Author

One of the remote data test fails is real: https://github.com/sunpy/sunpy/runs/5557145861?check_suite_focus=true#step:7:5237 and I've just rebooted the figure tests to check them (I think the fail was the MPL issue that's now fixed)

Ah thanks for catching that. I've ignored the warnings in the two doctests. We'll also need to remove that section of the guide once the coalignment stuff actually gets removed.

@nabobalis nabobalis merged commit e45b9b2 into sunpy:main Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
image Affects the image 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.

3 participants