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

Correct y limits in ProfileViewerState when min same as max #2513

Merged
merged 9 commits into from
Sep 24, 2024

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Sep 11, 2024

This fixes a display limit problem we see over at Cubeviz when cube is all ones.

import numpy as np
from astropy import units as u
from specutils import Spectrum1D

from jdaviz import Cubeviz

sp = Spectrum1D(flux=np.ones((7, 8, 9)) * u.nJy)

cubeviz_helper = Cubeviz()
cubeviz_helper.load_data(sp, data_label='test_cube')
cubeviz_helper.show()

Without this patch:

Screenshot 2024-09-11 155700

With this patch:

Screenshot 2024-09-11 155442

🐱

Copy link
Collaborator

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. A good choice for a default range still is somewhat arbitrary, since I cannot conceive any real data where this case would apply. But I've tested this branch with some other examples, and setting a range of at least a few percents would probably be good to have it visible in the tick marks.
I think it should be possible to extend test_limits to cover this case?

glue/viewers/profile/state.py Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

@dhomeier good point about very small values. I updated the code. How does it look now?

@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

Where is test_limits? I cannot find it.

@dhomeier
Copy link
Collaborator

dhomeier commented Sep 23, 2024

I had just edited the suggestion to a slightly different version to account for negative values as well, which is probably now hidden in the resolved.

@dhomeier
Copy link
Collaborator

def test_limits(self):

@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

@dhomeier , can u please re-suggest the neg fix so commit will credit you properly? Also feel free to push directly to this branch if that is easier. Thanks!

@dhomeier dhomeier changed the title BUG: Correct y limits when min same as max Correct y limits in ProfileViewerState when min same as max Sep 23, 2024
@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

I don't think the Windows job failures are related?

@dhomeier
Copy link
Collaborator

Suggestion is easier for now, but if you don't find time to write a test, let me know, and I can probably look into that later.

@dhomeier
Copy link
Collaborator

No, the image loader failures I have seen elsewhere.

@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

Added a couple of tests. Please re-review. Thanks!

@dhomeier
Copy link
Collaborator

dhomeier commented Sep 23, 2024

Thanks! I'd parametrise that test, but cannot push to your branch.

I have pushed to my own fork, so could open a PR against this, phew.

@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

Hmm I enabled push by maintainer, so weird it won't let you push. Did you work off my branch? If you rebase, then that might mess up the history, and not let you push without a force.

@dhomeier
Copy link
Collaborator

No, I had reset to e9df453, so should be on the same history, and it is not failing on that afaict:

To github.com:pllim/glue.git
! [remote rejected] pllim-fix-yminmax-same -> pllim-fix-yminmax-same (permission denied)

@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

PR to my branch works too. Pls ping me there. Thx

@dhomeier
Copy link
Collaborator

pllim#1

@dhomeier
Copy link
Collaborator

I created the feature branch from glue-viz/glue main, not sure if that matters.

@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

@dhomeier , I merged your PR to my PR, so your changes is in this PR now. Thanks!

@dhomeier
Copy link
Collaborator

Thanks, LGTM now!

@pllim
Copy link
Contributor Author

pllim commented Sep 23, 2024

Thanks for your help!

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Thanks!

@astrofrog astrofrog merged commit 1c51c7f into glue-viz:main Sep 24, 2024
24 of 25 checks passed
@pllim pllim deleted the fix-yminmax-same branch September 24, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants