spacetelescope / jdaviz

JWST astronomical data analysis tools in the Jupyter platform
https://jdaviz.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
138 stars 73 forks source link

Debug cubeviz spectrum1d parser #3133

Closed rosteen closed 1 month ago

rosteen commented 2 months ago

Fixes #3117

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

pllim commented 2 months ago

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 commented 2 months ago

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)?

rosteen commented 2 months ago

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)?

codecov[bot] commented 2 months ago

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 25 commits behind head on main.

Additional details and impacted files ```diff @@ 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.

pllim commented 2 months ago

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

Yes, I think that is better. Thanks!

rosteen commented 2 months ago

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 commented 1 month ago

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

lumberbot-app[bot] commented 1 month ago

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
  2. 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.

pllim commented 1 month ago

@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.

rosteen commented 1 month ago

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