jdaviz icon indicating copy to clipboard operation
jdaviz copied to clipboard

Debug cubeviz spectrum1d parser

Open rosteen opened this issue 1 year ago • 6 comments

Fixes #3117

Opening as draft since I still need to address the aperture photometry test failures.

rosteen avatar Aug 05 '24 14:08 rosteen

But we assume it is flipped in the mouseover. You fixed the flipping but not mouseover code, but CI still passes. What does this mean?

pllim avatar Aug 05 '24 18:08 pllim

Also, can someone check that we're not re-introducing the problem where the display in Cubeviz is transposed compared to a third-part cube viewer (I forgot the name)?

pllim avatar Aug 05 '24 18:08 pllim

But we assume it is flipped in the mouseover. You fixed the flipping but not mouseover code, but CI still passes. What does this mean?

Well, for one, this only affects loading Spectrum1D objects, not loading from file. So anything using remote data is unaffected. And it looks like the mouseover tests in Cubeviz that would have been affected are looking at pixel (1, 1) and thus immune to flipping. Think I should update at least one of those to look at something non-symmetric like (1, 2)?

rosteen avatar Aug 05 '24 18:08 rosteen

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.82%. Comparing base (3de1b08) to head (e53d128). Report is 156 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3133      +/-   ##
==========================================
- Coverage   88.91%   88.82%   -0.09%     
==========================================
  Files         111      112       +1     
  Lines       17365    17429      +64     
==========================================
+ Hits        15440    15482      +42     
- Misses       1925     1947      +22     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 05 '24 18:08 codecov[bot]

look at something non-symmetric like (1, 2)?

Yes, I think that is better. Thanks!

pllim avatar Aug 05 '24 19:08 pllim

look at something non-symmetric like (1, 2)?

Yes, I think that is better. Thanks!

I updated one of the mouseover tests to be sensitive to an axis swap.

rosteen avatar Aug 05 '24 19:08 rosteen

Fixed that comment, I'll merge after tests pass.

rosteen avatar Aug 08 '24 15:08 rosteen

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 v3.10.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 b9b4b1a3b1b0991dc687353cee6fdfd02bc05c67
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #3133: Debug cubeviz spectrum1d parser'
  1. Push to a named branch:
git push YOURFORK v3.10.x:auto-backport-of-pr-3133-on-v3.10.x
  1. Create a PR against branch v3.10.x, I would have named this PR:

"Backport PR #3133 on branch v3.10.x (Debug cubeviz spectrum1d parser)"

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[bot] avatar Aug 08 '24 16:08 lumberbot-app[bot]

@rosteen , can you please manually backport or change the milestone? If there is no plan to do another bug fix release, not much point in backporting.

pllim avatar Aug 08 '24 17:08 pllim

Hmm, since there are no other bugfixes waiting to go out, I'll re-milestone to 4.0.

rosteen avatar Aug 08 '24 17:08 rosteen