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

Fix submap not using all four corners of the input rectangle #4727

Merged
merged 5 commits into from
Dec 17, 2020

Conversation

Cadair
Copy link
Member

@Cadair Cadair commented Dec 11, 2020

Fixes #4725
Fixes #4734

@Cadair Cadair requested a review from a team as a code owner December 11, 2020 17:31
@Cadair Cadair added [BugFix] Needs Review Needs reviews before merge. map Affects the map submodule Priority High Rapid action required. labels Dec 11, 2020
@Cadair Cadair requested a review from a team as a code owner December 11, 2020 17:51
@Cadair Cadair removed the request for review from a team December 11, 2020 17:57
changelog/4727.bugfix.rst Outdated Show resolved Hide resolved
sunpy/map/mapbase.py Outdated Show resolved Hide resolved
changelog/4727.bugfix.rst Outdated Show resolved Hide resolved
Copy link

@kakirastern kakirastern left a comment

Choose a reason for hiding this comment

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

About all set to go, pending like some faster RTD docs build debugging...

@kakirastern

This comment has been minimized.

sunpy/map/mapbase.py Outdated Show resolved Hide resolved
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.

Looks 👍 apart from

  • Removing the new test file
  • Updating the changelog, stating clearly how users can change code to adapt to this change

submap now once again correctly identifies the smallest pixel box which contains
all four corners of the input rectangle.
@Cadair
Copy link
Member Author

Cadair commented Dec 15, 2020

I have upgraded the changelog entry to breaking, mainly to put it towards the top of the changelog for 2.1 so people see it, also added the code to restore behaviour. I have removed the roll test file and update the fixture.

@Cadair
Copy link
Member Author

Cadair commented Dec 15, 2020

test fails are real.

@hayesla
Copy link
Member

hayesla commented Dec 15, 2020

this PR is hurting me brain - good catch on that bug

Does this change what submap returns for cases where wcs axes and pixel axes are perfectly aligned?

@Cadair
Copy link
Member Author

Cadair commented Dec 15, 2020

Does this change what submap returns for cases where wcs axes and pixel axes are perfectly aligned?

No in this case the assumption holds and the results will be the same.

@Cadair
Copy link
Member Author

Cadair commented Dec 15, 2020

this PR is hurting me brain

Yeha, mine too. Figuring out the regression test really hurt.

Copy link
Member

@hayesla hayesla left a comment

Choose a reason for hiding this comment

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

looks good!

That test fail is fair, and probably expected (array size makes more sense now I think..) - the docsting just needs updating

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.

Woops, sorry for missing the failing test, will mark as needs changes until it's fixed

@Cadair
Copy link
Member Author

Cadair commented Dec 15, 2020

I have updated the doctests and found a bug in the process. I was assuming that the frame was HPC. I have now rectified this, added a test for using HGS coords with a map in HPC and HGS and also added a mask test for good measure.

@Cadair Cadair requested a review from dstansby December 15, 2020 15:12
@Cadair Cadair merged commit f80ec7a into sunpy:master Dec 17, 2020
@Cadair Cadair deleted the submap_regression branch December 17, 2020 13:50
@sunpy-backport
Copy link

The backport to 2.0 failed:

Commits ["9ec00247c34dc503ead49f1bae763ebc8ced1b2e","cb23d08ca8e8c1167a651f51e7f4f6fbd517aded","df1324426d4b4f20eebfb342642af611569a58f0","bf7f1b334e8414841e3914837694853b60aca75b","f99faf6855868f3ca84bca9607b2b56d502dd826"] could not be cherry-picked on top of 2.0

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 2.0
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 9ec00247c34dc503ead49f1bae763ebc8ced1b2e cb23d08ca8e8c1167a651f51e7f4f6fbd517aded df1324426d4b4f20eebfb342642af611569a58f0 bf7f1b334e8414841e3914837694853b60aca75b f99faf6855868f3ca84bca9607b2b56d502dd826
# Create a new branch with these backported commits.
git checkout -b backport-4727-to-2.0
# Push it to GitHub.
git push --set-upstream origin backport-4727-to-2.0
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport-4727-to-2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map Affects the map submodule Priority High Rapid action required.
Projects
None yet
6 participants