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

Flip jp2 data vertically before saving #7486

Merged
merged 5 commits into from Mar 20, 2024
Merged

Conversation

dgarciabriseno
Copy link
Contributor

PR Description

Fixes #7485

Flipping the data array when saving as jp2 fixes this problem.

@dgarciabriseno dgarciabriseno requested a review from a team as a code owner March 8, 2024 17:41
@dgarciabriseno
Copy link
Contributor Author

test

ayshih
ayshih previously requested changes Mar 8, 2024
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.

You need to fix the relevant unit test (

# Sanity check that reading back the jp2 returns coherent data
jp2_readback = _jp2.read(outfile)
assert header['DATE'] == jp2_readback[0].header['DATE']
) so that it actually verifies the output

sunpy/io/_jp2.py Outdated
@@ -176,7 +176,7 @@ def write(fname, data, header, **kwargs):

tmpname = fname + "tmp.jp2"
jp2_data = np.uint8(data)
jp2 = Jp2k(tmpname, jp2_data, **kwargs)
jp2 = Jp2k(tmpname, np.flip(jp2_data, 0), **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment that explains that we have to flip the array back here because we flip it upon reading (due to the difference in the origin of the data)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where does it get flipped when its read in?
Oh it's the [::-1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment here, and updated the test to verify the data array after reading it back.

@dgarciabriseno
Copy link
Contributor Author

dgarciabriseno commented Mar 8, 2024

You need to fix the relevant unit test (

# Sanity check that reading back the jp2 returns coherent data
jp2_readback = _jp2.read(outfile)
assert header['DATE'] == jp2_readback[0].header['DATE']

) so that it actually verifies the output

How would you recommend doing that?
I'm not sure the result will match the original fits data array since it casts the data to uint8 to save it as a jp2. (There's other issues with doing that too, a simple cast isn't sufficient to convert from fits to jp2).

edit:

  • I suppose I could cast the original data to the uint8 array and then compare it.

sunpy/io/_jp2.py Outdated Show resolved Hide resolved
Copy link
Member

@wtbarnes wtbarnes left a comment

Choose a reason for hiding this comment

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

Just needs a changelog, but other than that looks good to me.

@wtbarnes wtbarnes requested a review from ayshih March 14, 2024 18:43
@wtbarnes wtbarnes added Needs Review Needs reviews before merge backport 5.0 on-merge: backport to 5.0 backport 5.1 on-merge: backport to 5.1 io Affects the io submodule labels Mar 14, 2024
@nabobalis nabobalis removed the Needs Review Needs reviews before merge label Mar 20, 2024
@nabobalis nabobalis dismissed ayshih’s stale review March 20, 2024 21:50

Fixed now hopefully

@nabobalis nabobalis merged commit 76c3ced into sunpy:main Mar 20, 2024
26 of 27 checks passed
Copy link

lumberbot-app bot commented Mar 20, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 5.0
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 76c3cedb9da116309eae5813db6d8987552c8317
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #7486: Flip jp2 data vertically before saving'
  1. Push to a named branch:
git push YOURFORK 5.0:auto-backport-of-pr-7486-on-5.0
  1. Create a PR against branch 5.0, I would have named this PR:

"Backport PR #7486 on branch 5.0 (Flip jp2 data vertically before saving)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@lumberbot-app lumberbot-app bot added the Still Needs Manual Backport This PR needs manually backporting label Mar 20, 2024
meeseeksmachine pushed a commit to meeseeksmachine/sunpy that referenced this pull request Mar 20, 2024
nabobalis added a commit that referenced this pull request Mar 20, 2024
…6-on-5.1

Backport PR #7486 on branch 5.1 (Flip jp2 data vertically before saving)
@nabobalis nabobalis removed the Still Needs Manual Backport This PR needs manually backporting label Apr 4, 2024
nabobalis added a commit to nabobalis/sunpy that referenced this pull request Apr 4, 2024
Flip jp2 data vertically before saving

(cherry picked from commit 76c3ced)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 5.0 on-merge: backport to 5.0 backport 5.1 on-merge: backport to 5.1 io Affects the io submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saving with the ".jp2" extension results in a vertically flipped image
4 participants