Get dev tests to pass #93

rosteen · 2024-03-05T21:59:23Z

Trying to get tests to pass with the dev version of Jdaviz. I suspect the axis rounding thing is actually the dev version of another dependency but I'm not sure.

codecov · 2024-03-05T22:03:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.18%. Comparing base (db3548d) to head (5956501).

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #93   +/-   ##
=======================================
  Coverage   94.18%   94.18%           
=======================================
  Files          35       35           
  Lines        1548     1548           
=======================================
  Hits         1458     1458           
  Misses         90       90

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

rosteen · 2024-03-06T16:54:38Z

I realized that the lcviz y_max test is failing with remote data because even though that particular test doesn't have remote data, the viewer still has the y_max from the remote data initially, so checking against that after resetting the zoom to the generated data obviously fails.

rosteen · 2024-03-06T17:13:43Z

I realized that the lcviz y_max test is failing with remote data because even though that particular test doesn't have remote data, the viewer still has the y_max from the remote data initially, so checking against that after resetting the zoom to the generated data obviously fails.

Seems like this is from spacetelescope/jdaviz#2649, after testing individual jdaviz commits.

rosteen · 2024-03-07T20:12:53Z

After more investigation I've pinned this down more precisely. Setting the bounds to the incorrect values (apparently from previous data) happens when the TimeScatterViewer calls super().add_data(). The call to state.reset_limits() after that changes the y_min and y_max, but the y_max appears to still be picking up the old (remote) data:

After super.add_data():  16095.5 16569.932734375
After set_plot_axes():  16095.5 16569.932734375
After state.reset_limits():  0.9675873265993092 16570.0

And actually, inside the with_delay_callback() block in reset_limits I can see that self.y_max has the correct value, but apparently that doesn't stick to the viewer. I wonder if the last thing getting set in that block is getting dropped somehow?

pllim · 2024-03-11T18:37:32Z

So I don't have to keep redigging.

lcviz/lcviz/viewers.py

Line 181 in 7b78df5

result = super().add_data(data, color, alpha, **layer_state)

Failure log: https://github.com/spacetelescope/lcviz/actions/runs/8050877356/job/21987497689

_____________________________ test_plugin_markers ______________________________

helper = <lcviz.helper.LCviz object at 0x7f51486d3850>
light_curve_like_kepler_quarter = <LightCurve length=4512 FLUX_ORIGIN=flux:orig>
       time               flux        ...     flux:orig      flux:orig_...1169425 ... 0.9881300661169425          0.01
 2455832.979167367  1.010578141808208 ...  1.010578141808208          0.01

    def test_plugin_markers(helper, light_curve_like_kepler_quarter):
        helper.load_data(light_curve_like_kepler_quarter)
        tv = helper.app.get_viewer(helper._default_time_viewer_reference_name)
    
        mp = helper.plugins['Markers']
        label_mouseover = mp._obj.coords_info
        mp.open_in_tray()
    
        # test event in flux-vs-time viewer
        label_mouseover._viewer_mouse_event(tv,
                                            {'event': 'mousemove',
                                             'domain': {'x': 0, 'y': 0}})
    
>       assert label_mouseover.as_text() == ('Cursor 0.00000e+00, 0.00000e+00',
                                             'Time 5.45833e+00 d',
                                             'Flux 9.67587e-01')
E       AssertionError: assert ('Cursor 0.00... 1.00497e+00') == ('Cursor 0.00... 9.67587e-01')
E         
E         At index 1 diff: 'Time 0.00000e+00 d' != 'Time 5.45833e+00 d'
E         
E         Full diff:
E           (
E               'Cursor 0.00000e+00, 0.00000e+00',
E         -     'Time 5.45833e+00 d',
E         ?           ^ ^^^^^
E         +     'Time 0.00000e+00 d',
E         ?           ^ ^^^^^
E         -     'Flux 9.67587e-01',
E         +     'Flux 1.00497e+00',
E           )

lcviz/tests/test_plugin_markers.py:49: AssertionError
______________________________ test_reset_limits _______________________________

helper = <lcviz.helper.LCviz object at 0x7f514575fdd0>
light_curve_like_kepler_quarter = <LightCurve length=4512 FLUX_ORIGIN=flux:orig>
       time               flux        ...     flux:orig      flux:orig_...1169425 ... 0.9881300661169425          0.01
 2455832.979167367  1.010578141808208 ...  1.010578141808208          0.01

    def test_reset_limits(helper, light_curve_like_kepler_quarter):
        helper.load_data(light_curve_like_kepler_quarter)
        tv = helper.app.get_viewer(helper._default_time_viewer_reference_name)
    
        orig_xlims = (tv.state.x_min, tv.state.x_max)
        orig_ylims = (tv.state.y_min, tv.state.y_max)
        # set xmin and ymin to midpoints
        new_xmin = (tv.state.x_min + tv.state.x_max) / 2
        new_ymin = (tv.state.y_min + tv.state.y_max) / 2
        tv.state.x_min = new_xmin
        tv.state.y_min = new_ymin
    
        tv.state._reset_x_limits()
        assert tv.state.x_min == orig_xlims[0]
        assert tv.state.y_min == new_ymin
    
        tv.state._reset_y_limits()
>       assert tv.state.y_min == orig_ylims[0]
E       assert 0.9675873265993092 == 0.0
E        +  where 0.9675873265993092 = <ScatterViewerState\n  angle_unit: radians\n  aspect: auto\n  dpi: 72\n  layers: <CallbackList with 1 elements>\n  plot_mod...y_limits_percentile: 100\n  y_log: False\n  y_max: 1.0392623770643632\n  y_min: 0.9675873265993092\n  y_ticklabel_size: 8\n>.y_min
E        +    where <ScatterViewerState\n  angle_unit: radians\n  aspect: auto\n  dpi: 72\n  layers: <CallbackList with 1 elements>\n  plot_mod...y_limits_percentile: 100\n  y_log: False\n  y_max: 1.0392623770643632\n  y_min: 0.9675873265993092\n  y_ticklabel_size: 8\n> = <lcviz.viewers.TimeScatterView object at 0x7f51457132d0>.state

lcviz/tests/test_viewers.py:19: AssertionError
=========================== short test summary info ============================
FAILED lcviz/tests/test_plugin_markers.py::test_plugin_markers - AssertionError: assert ('Cursor 0.00... 1.00497e+00') == ('Cursor 0.00... 9.67587e-01')
  
  At index 1 diff: 'Time 0.00000e+00 d' != 'Time 5.45833e+00 d'
  
  Full diff:
    (
        'Cursor 0.00000e+00, 0.00000e+00',
  -     'Time 5.45833e+00 d',
  ?           ^ ^^^^^
  +     'Time 0.00000e+00 d',
  ?           ^ ^^^^^
  -     'Flux 9.67587e-01',
  +     'Flux 1.00497e+00',
    )
FAILED lcviz/tests/test_viewers.py::test_reset_limits - assert 0.9675873265993092 == 0.0
 +  where 0.9675873265993092 = <ScatterViewerState\n  angle_unit: radians\n  aspect: auto\n  dpi: 72\n  layers: <CallbackList with 1 elements>\n  plot_mod...y_limits_percentile: 100\n  y_log: False\n  y_max: 1.0392623770643632\n  y_min: 0.9675873265993092\n  y_ticklabel_size: 8\n>.y_min
 +    where <ScatterViewerState\n  angle_unit: radians\n  aspect: auto\n  dpi: 72\n  layers: <CallbackList with 1 elements>\n  plot_mod...y_limits_percentile: 100\n  y_log: False\n  y_max: 1.0392623770643632\n  y_min: 0.9675873265993092\n  y_ticklabel_size: 8\n> = <lcviz.viewers.TimeScatterView object at 0x7f51457132d0>.state

pllim · 2024-03-11T18:56:39Z

UPDATE: Nevermind, I don't think freezable anything is used here. See spacetelescope/jdaviz#2746

My hunch points to https://github.com/spacetelescope/jdaviz/blob/0655d4a43b82323042db87a74d352ecf0590d5fc/jdaviz/core/freezable_state.py#L70-L71 but I dunno what that code is supposed to accomplish. When all the limits are being changed at once, won't _set_axes_lim be called too many times?

jdaviz/blob/main/jdaviz/core/freezable_state.py

class FreezableBqplotImageViewerState(BqplotImageViewerState, FreezableState):
# ...
    def __init__(self, *args, **kwargs):
        # ...
        for attr in ('x_min', 'x_max', 'y_min', 'y_max'):
            self.add_callback(attr, self._set_axes_lim)
        super().__init__(*args, **kwargs)

reset_limits likely goes all the way here.

https://github.com/glue-viz/glue/blob/1bbd6e8282a59abfc857eab70f2c9cffe455bc05/glue/viewers/scatter/state.py#L74-L89

    def reset_limits(self):
        if not self.using_polar:
            self._reset_x_limits()
        self._reset_y_limits()

Also there is no example of setting state.x_min etc directly that I can see in https://github.com/glue-viz/glue/blob/main/glue/viewers/scatter/tests/test_viewer.py

kecnry · 2024-03-11T19:06:04Z

Yes, that can result in the same code being needlessly called 4 times if not delayed when calling, but the final call should contain the final limits and result in the correct result. This should not apply here though since that is only used for image viewers.

pllim · 2024-03-11T19:10:11Z

Actually, I see a pattern. In Jdaviz, all the viewers has something like this:

_state_cls = FreezableSomethingViewerState

But not here:

lcviz/lcviz/viewers.py

Line 37 in 7b78df5

_state_cls = ScatterViewerState

But since Jdaviz does not have any scatter viewer, there is no such thing as a FreezableScatterViewerState. Is this a problem?

kecnry · 2024-03-11T19:16:42Z

No, lcviz does not need any overrides to the state at this time (the name "Freezable" is for mosviz, which does not apply here, and none of the other overrides to reset_limits, etc, are needed here). I don't suspect that would be the cause of this bug (and wasn't changed in the at-fault PR) - but I could be wrong since I don't know what is actually causing the failure.

pllim · 2024-03-11T21:05:36Z

So your scatter viewer state is here: https://github.com/glue-viz/glue/blob/1bbd6e8282a59abfc857eab70f2c9cffe455bc05/glue/viewers/scatter/state.py#L23C1-L23C52

class ScatterViewerState(MatplotlibDataViewerState)

The parent is here: https://github.com/glue-viz/glue/blob/1bbd6e8282a59abfc857eab70f2c9cffe455bc05/glue/viewers/matplotlib/state.py#L118 (I guess the Matplotlib in the name is misleading since glue-jupyter also uses it)

class MatplotlibDataViewerState(ViewerState):
    """
    A base class that includes common attributes for viewers based on
    Matplotlib.
    """

    x_min = DeferredDrawCallbackProperty(docstring='Lower limit of the visible x range')
    x_max = DeferredDrawCallbackProperty(docstring='Upper limit of the visible x range')

    y_min = DeferredDrawCallbackProperty(docstring='Lower limit of the visible y range')
    y_max = DeferredDrawCallbackProperty(docstring='Upper limit of the visible y range')

The attributes you are changing (x/y_min/max) are all class attributes. Theoretically this means, they are shared across instances.

When the parent changes it directly, a child might be affected. Consider this:

class Parent:
    x_min = 1

class Child(Parent):
    pass

c1 = Child()
c2 = Child()

>>> c2.x_min = 1
>>> Parent.x_min = 100
>>> c1.x_min  # Inherits from Parent class, is this what is happening?
100
>>> c2.x_min  # Already set in instance before, so not affected
1

pllim · 2024-03-11T21:09:23Z

To continue from above, it looks like the scatter viewer state resets using percentile, unlike the profile viewer, and yes, the percentile is a class attribute. So if you never touch it here, then maybe percentile is being changed at the class level accidentally?

https://github.com/glue-viz/glue/blob/1bbd6e8282a59abfc857eab70f2c9cffe455bc05/glue/viewers/scatter/state.py#L80C1-L84C52

    def _reset_y_limits(self, *args):
        if self.y_att is None:
            return
        self.y_lim_helper.percentile = self.y_limits_percentile
        self.y_lim_helper.update_values(force=True)

pllim · 2024-03-12T18:01:25Z

Might have found the culprit: https://github.com/spacetelescope/lcviz/pull/96/files#r1521909661

Compare y_max, which doesn't get rounded by viewer (?)

5956501

pllim mentioned this pull request Mar 11, 2024

Add test similar to lcviz spacetelescope/jdaviz#2747

Merged

9 tasks

This was referenced Mar 12, 2024

TST: See if percentile is breaking devdeps #95

Closed

TST: See if percentile is breaking devdeps (try 2) #96

Closed

rosteen closed this Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get dev tests to pass #93

Get dev tests to pass #93

rosteen commented Mar 5, 2024

codecov bot commented Mar 5, 2024

rosteen commented Mar 6, 2024 •

edited

Loading

rosteen commented Mar 6, 2024

rosteen commented Mar 7, 2024 •

edited

Loading

pllim commented Mar 11, 2024 •

edited

Loading

pllim commented Mar 11, 2024 •

edited

Loading

kecnry commented Mar 11, 2024

pllim commented Mar 11, 2024

kecnry commented Mar 11, 2024 •

edited

Loading

pllim commented Mar 11, 2024

pllim commented Mar 11, 2024

pllim commented Mar 12, 2024

Get dev tests to pass #93

Get dev tests to pass #93

Conversation

rosteen commented Mar 5, 2024

codecov bot commented Mar 5, 2024

Codecov Report

rosteen commented Mar 6, 2024 • edited Loading

rosteen commented Mar 6, 2024

rosteen commented Mar 7, 2024 • edited Loading

pllim commented Mar 11, 2024 • edited Loading

pllim commented Mar 11, 2024 • edited Loading

kecnry commented Mar 11, 2024

pllim commented Mar 11, 2024

kecnry commented Mar 11, 2024 • edited Loading

pllim commented Mar 11, 2024

pllim commented Mar 11, 2024

pllim commented Mar 12, 2024

rosteen commented Mar 6, 2024 •

edited

Loading

rosteen commented Mar 7, 2024 •

edited

Loading

pllim commented Mar 11, 2024 •

edited

Loading

pllim commented Mar 11, 2024 •

edited

Loading

kecnry commented Mar 11, 2024 •

edited

Loading