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

Allow CategoricalComponent to be n-dimensional #2214

Merged
merged 5 commits into from
Aug 12, 2021

Conversation

astrofrog
Copy link
Member

@jfoster17 - can you check if this works properly for you?

Fixes #2213

@jfoster17
Copy link
Member

What is your preferred way for me to make additions to this? A PR into astrofrog:categorical-nd, or just pasting here?

You need to update core.component.autotyped

n = coerce_numeric(data.ravel()).reshape(data.shape)

Proposed addition to test_component.py

def test_nd_categorical_component_autotype():
    x = np.array([['M', 'F'], ['F', 'M']])
    assert Component.autotyped(x).categorical

    d = Data(x=[['male', 'female', 'male'],['female','female','male']])
    assert d.get_component('x').categorical

@jfoster17
Copy link
Member

This still doesn't do exactly what I would want to it do when you combine a 2D image of some matrix data with a 1D histogram of the categorical components. Specifically, making a subset on the 2D image does make a selection on the categorical array that properly updates the histogram but trying to make a subset on the 1D histogram to (for example) select all the points in the 2D image with a given categorical value does not work:

  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/backends/backend_qt5.py", line 480, in _draw_idle
    self.draw()
  File "/Users/jfoster/glue/glue/viewers/matplotlib/qt/widget.py", line 124, in draw
    return super(MplCanvas, self).draw(*args, **kwargs)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/backends/backend_agg.py", line 407, in draw
    self.figure.draw(self.renderer)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/artist.py", line 41, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/figure.py", line 1863, in draw
    mimage._draw_list_compositing_images(
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/image.py", line 131, in _draw_list_compositing_images
    a.draw(renderer)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/astropy/visualization/wcsaxes/core.py", line 459, in draw
    super().draw(renderer, **kwargs)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/artist.py", line 41, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/cbook/deprecation.py", line 411, in wrapper
    return func(*inner_args, **inner_kwargs)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/axes/_base.py", line 2747, in draw
    mimage._draw_list_compositing_images(renderer, self, artists)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/image.py", line 131, in _draw_list_compositing_images
    a.draw(renderer)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/artist.py", line 41, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/matplotlib/image.py", line 643, in draw
    im, l, b, trans = self.make_image(
  File "/Users/jfoster/opt/anaconda3/envs/glueviz-dev/lib/python3.9/site-packages/mpl_scatter_density/base_image_artist.py", line 177, in make_image
    array = self._array_func(bins=bins, range=((ymin, ymax), (xmin, xmax)))
  File "/Users/jfoster/glue/glue/viewers/image/frb_artist.py", line 27, in array_func_wrapper
    array = self._array_maker(bounds)
  File "/Users/jfoster/glue/glue/viewers/image/layer_artist.py", line 245, in __call__
    mask = self.layer_state.get_sliced_data(bounds=bounds)
  File "/Users/jfoster/glue/glue/viewers/image/state.py", line 455, in get_sliced_data
    image = self.layer.data.compute_fixed_resolution_buffer(full_view, target_data=self.viewer_state.reference_data,
  File "/Users/jfoster/glue/glue/core/data.py", line 1945, in compute_fixed_resolution_buffer
    return compute_fixed_resolution_buffer(self, *args, **kwargs)
  File "/Users/jfoster/glue/glue/core/fixed_resolution_buffer.py", line 274, in compute_fixed_resolution_buffer
    array[invalid_all] = invalid_value
IndexError: too many indices for array: array is 1-dimensional, but 2 were indexed

The interaction I describe above does work if the 2D component is, e.g. a 2D array of 1/0 instead of a full CategoricalComponent.

@jfoster17
Copy link
Member

The above error message is a bit leading, it is actually coming from the 2D image viewer I had open.

The core problem actually seems to be that selecting a portion of a categorical 1D histogram where the underlying data is >1D leads to a 1D subset (at least with to_mask()).

NB: this will only work with my fix to autotyped posted above.

from glue.core import Data
data = Data(x=[['Male','Female','Male'],['Female','Male','Male']],label='test')
dc.append(data)

I then create a 1D histogram and try to select the Females. This creates a subset without issuing an error, but the subset does not display. This is probably related to the fact that:

In [11]: dc[0].subsets[0].to_mask()
Out[11]: array([False,  True, False,  True, False, False])

Whereas doing it at the command line does the correct thing:

state = data.id['x'] == 'Male'
subset_group = dc.new_subset_group('Male',state)
dc[0].subsets[1].to_mask()

array([[ True, False,  True],
       [False,  True,  True]])

@jfoster17
Copy link
Member

I think I have fixed the masking problem with Nd categoricals. The branch/fork here:

https://github.com/gluesolutions/glue/tree/categorical-nd

Shows that solution (just remove a .ravel() statement) and includes a test that failed before and now works.

@astrofrog
Copy link
Member Author

@jfoster17 - thanks! I've rebased this and included your change here so let's see if everything passes now.

@astrofrog astrofrog closed this Aug 10, 2021
@astrofrog astrofrog reopened this Aug 10, 2021
@astrofrog
Copy link
Member Author

astrofrog commented Aug 12, 2021

The CI issues are real, somehow some of the changes here are causing a massive slowdown somewhere which is causing the CI to time out. Trying to determine where this is happening.

@astrofrog
Copy link
Member Author

I think I figured it out, let's see what the CI says

@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #2214 (2920829) into master (da6693c) will decrease coverage by 0.03%.
The diff coverage is 80.95%.

❗ Current head 2920829 differs from pull request most recent head dbc27db. Consider uploading reports for the commit dbc27db to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2214      +/-   ##
==========================================
- Coverage   88.05%   88.01%   -0.04%     
==========================================
  Files         247      247              
  Lines       23168    23167       -1     
==========================================
- Hits        20401    20391      -10     
- Misses       2767     2776       +9     
Impacted Files Coverage Δ
glue/core/data_collection.py 88.34% <73.33%> (-1.99%) ⬇️
glue/core/component.py 95.43% <100.00%> (-0.83%) ⬇️
glue/core/subset.py 89.82% <100.00%> (ø)
glue/utils/array.py 89.49% <100.00%> (ø)
glue/viewers/matplotlib/qt/widget.py 73.86% <0.00%> (-3.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 049d3c3...dbc27db. Read the comment docs.

@astrofrog astrofrog merged commit 5978a92 into glue-viz:master Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow categorical components to be >1D
2 participants