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

Debug cubeviz spectrum1d parser #3133

Merged

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Aug 5, 2024

Fixes #3117

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

@rosteen rosteen added bug Something isn't working cubeviz 💤backport-v3.10.x on-merge: backport to v3.10.x labels Aug 5, 2024
@rosteen rosteen added this to the 3.10.4 milestone Aug 5, 2024
@github-actions github-actions bot added the plugin Label for plugins common to multiple configurations label Aug 5, 2024
@rosteen rosteen marked this pull request as ready for review August 5, 2024 18:10
@pllim
Copy link
Contributor

pllim commented Aug 5, 2024

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
Copy link
Contributor

pllim commented Aug 5, 2024

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
Copy link
Collaborator Author

rosteen commented Aug 5, 2024

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

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pllim
Copy link
Contributor

pllim commented Aug 5, 2024

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

Yes, I think that is better. Thanks!

@rosteen
Copy link
Collaborator Author

rosteen commented Aug 5, 2024

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.

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

My head still hurts but this seems to fix things. Thanks!

Copy link
Contributor

@bmorris3 bmorris3 left a comment

Choose a reason for hiding this comment

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

This was a confusing one, thanks for the fix.

jdaviz/configs/cubeviz/plugins/tests/test_parsers.py Outdated Show resolved Hide resolved
@rosteen
Copy link
Collaborator Author

rosteen commented Aug 8, 2024

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

@rosteen rosteen merged commit b9b4b1a into spacetelescope:main Aug 8, 2024
18 of 19 checks passed
Copy link

lumberbot-app bot commented Aug 8, 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 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.

@pllim
Copy link
Contributor

pllim commented Aug 8, 2024

@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
Copy link
Collaborator Author

rosteen commented Aug 8, 2024

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

@rosteen rosteen removed the 💤backport-v3.10.x on-merge: backport to v3.10.x label Aug 8, 2024
@rosteen rosteen modified the milestones: 3.10.4, 4.0 Aug 8, 2024
pllim added a commit to cshanahan1/jdaviz that referenced this pull request Aug 9, 2024
pllim added a commit that referenced this pull request Aug 9, 2024
* aperture photometry plugin listens to unit conversion

* pllim minor edits

review comment

Allow per-wave to/from per-freq for most cases

Co-authored-by:  Clare Shanahan <cshanahan@stsci.edu>

* TST: Update result that changed
because of #3133

---------

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cubeviz plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Wrong WCS mapping when reading from Spectrum1D
4 participants