-
Notifications
You must be signed in to change notification settings - Fork 74
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
Speed up histogram in plot options (alternative to #2763) #2771
Changes from all commits
663eac6
0a6aea2
ee24ab9
394f6ac
16de08f
5da50e4
b4f4049
46d376f
197789b
b814828
30a4707
0bc72d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,11 @@ def test_stretch_bounds(imviz_helper): | |
|
||
|
||
def test_stretch_bounds_and_spline(imviz_helper): | ||
|
||
# As the histogram randomly samples the array, we should make sure the | ||
# values used here are reproducible | ||
np.random.seed(42) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm is there no way to localize it more within the histogram algorithm itself? Maybe something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we actually want to do that right? it's more just for testing? In practice it's probably safer to not constrain the random numbers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you want to also set seed for testing on glue side? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes but I can always do that in the relevant glue tests if needed. I think there shouldn't be a seed set at runtime. |
||
|
||
image_1 = NDData(make_4gaussians_image(), unit=u.nJy) | ||
imviz_helper.load_data(image_1) | ||
po = imviz_helper.plugins["Plot Options"] | ||
|
@@ -93,7 +98,7 @@ def test_stretch_bounds_and_spline(imviz_helper): | |
|
||
knots_after_drag_move = ( | ||
[0.0, 0.1, 0.21712585033417825, 0.7, 1.0], | ||
[0.0, 0.05, 0.2900993441358025, 0.9, 1.0], | ||
[0.0, 0.05, 0.2852214046563617, 0.9, 1.0], | ||
) | ||
|
||
stretch_tool.on_mouse_event(knot_move_msg) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is hardcoding percentile here ok, since we also let users change percentile in Plot Options, i.e., would users care or notice anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was just writing something equivalent to what was there before, but we can easily change if you think users should be able to customise it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong opinion here. I always think of this histogram as quick look and should not be taken too seriously but @camipacifici or @kecnry might have different opinions, so let's see if they comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know what this specific line does exactly, so not sure how to comment, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is something we'll want to deal with eventually since it isn't always ideal, but this was hardcoded before so I think its reasonable to keep the same assumption for now