New Visual: Bar #2394

Bliss3d · 2022-10-02T14:32:11Z

This PR replaces #2287, this contains fixes for Bar.meshdata.get_vertices() which error out in the previous PR and is probebly a better solution and contains an example

As ever your input is very welcome :)

python.2022-10-02.16-07-02-03.mp4

djhoese · 2022-10-03T14:43:27Z

Wow, this seems very simple for such a nice feature. I'm wondering how you think this compares to some of the logic in the existing HistogramVisual:

https://github.com/vispy/vispy/blob/main/vispy/visuals/histogram.py

Could/should this new Visual be used by the HistogramVisual? I notice the HistogramVisual allows for orientation (even though it is currently backwards from my experiments, see #2372). How hard would it be to add that here?

djhoese · 2022-10-03T14:45:41Z

Also, don't worry about the (3.9, full, standard, true) test that is failing. That is the new PyQt6 causing seg faults.

Otherwise, I forgot to mention, would it be possible to add a unit test for this new visual? Maybe something simple that shows that it is able to draw. You could even check the colors at certain points in the rendered result. For example, if the first bar is should fill the whole SceneCanvas then make sure that a color near the top of the image is the right color. And if the last bar is 50% of the canvas then make sure the colors are correct at the top and bottom half of the image.

djhoese · 2022-10-03T14:46:21Z

examples/scene/bar_plot.py

@@ -0,0 +1,32 @@
+import numpy as np


This example will need a docstring like the other examples in this gallery directory. Formatting and content is important since it appears in the final website.

Bliss3d · 2022-10-03T15:44:52Z

Wow, this seems very simple for such a nice feature. I'm wondering how you think this compares to some of the logic in the existing HistogramVisual:

https://github.com/vispy/vispy/blob/main/vispy/visuals/histogram.py

Could/should this new Visual be used by the HistogramVisual? I notice the HistogramVisual allows for orientation (even though it is currently backwards from my experiments, see #2372). How hard would it be to add that here?

As I started with vispy ploting I found it very strage that there where no bar visuals even though there is a histogram visual, seems kind assbackwards, no offense. However it familiarized me with vispy ploting and I did use it to guide me to create #2287 - now I'm much more proficient with vispy ploting, I'm personally more interested in the Candle visual this is just a by-product. Short anwser is yes it would probebly make sense to use bars in the histogram. However I dont use histograms so I'm hardly an authority on the matter - people who actually use the histogram component should make the decision. If I remember correctly in a vispy histogram there is no option for spaces between bins and its a single mesh, which in my opinion looks kind of outdated. It might be useful to have a single mesh for a histogram as I sad I dont use histograms.

Regaarding vertical bars probebly just switch width and height

And for unit tests i've never done them, so I have no experience

djhoese · 2022-10-03T20:52:50Z

examples/scene/bar_plot.py

+# -----------------------------------------------------------------------------
+"""
+Bar plot with Axis 
+=====================


The underline must be the same number of characters (width) as the title.

Okay, fixed

Bliss3d · 2022-10-03T23:08:51Z

As per your request vertical bar ploting for a possible usage in histogram is now implemented, easy as pie

brisvag

Looks really good in general! I proposed some small change for clarity's sake, but as far as I can tell it makes sense. I really thionk we need some comments about what each part of calc_vertices is doing, othwerwise we won't know how to maintain it a month from now, since it's quite cryptic.

Other than that, I think we could merge this, and think about using it as our histogram visual at a later time!

brisvag · 2022-10-04T07:37:48Z

vispy/visuals/bar.py

+    def update_data(self, height, bottom=None, width=0.8, shift=0.0, color='w'):
+        if bottom is None:
+            bottom = np.zeros(height.shape[0])
+        rr, tris = calc_vertices(height, bottom, width, shift=shift)


Missing orientation here

Obviously yes, it was late yesterday

brisvag · 2022-10-04T07:38:44Z

vispy/visuals/bar.py

+        Orientation of the histogram - 'h' is default
+    """
+
+    def __init__(self, height, bottom=None, width=0.8, shift=0.0, color='w', orientation='h'):


Would be awesome to be able to pass a per-bar color as well!

I think I implemented something kinda like that in #2388 and those PR's are very samey.
#2388 has also more comments trying to explain whats going on

Yeah no need to do this here, I just pointed out it would be cool to add in the future :)

Well I just looked into it, it should be not to problematic

vispy/visuals/bar.py

brisvag · 2022-10-04T07:54:26Z

vispy/visuals/bar.py

+    vertices = np.array([
+        [-width/2 + shift, 1, 0],
+        [width/2 + shift, 1, 0],
+        [width/2 + shift, 0, 0],
+        [-width/2 + shift, 0, 0]])


What's the point of the 0 at index 2? I'm having a hard time following the flow.

Well I thought that vertices would have to be x,y,z and the resulting vertices for the mesh should be in the shape (n,3), since its a 2d plot the z axis is zero. I'm not married to it if you want to change it it would be fine with me

Ok, I see why I was confused: this resulted in a deprecation warning from numpy due to a staggered sequence in my testing, because I was passing an array to width rather than a single value... My bad :P

But yeah, I think you could remove the zero there and it would be fine, meshes work well with 2D data and we do it in several other places as well.

Only argument for not removing the zero would be if it avoids an unnecessary copy. Another thing to consider performance-wise would be to create one giant array at the beginning and fill it in later. This is typically faster since we don't have to keep allocating new numpy arrays, but may only be true for really large arrays.

brisvag · 2022-10-04T07:55:48Z

vispy/visuals/bar.py

+
+    base_faces = np.tile(base_faces, (height.shape[0], 1))
+
+    faces = stack_n_x2*4 + base_faces


ok, maybe this use of stack_n_x2 counters my proposal of using repeat(4) above, if I'm following.

brisvag · 2022-10-04T09:08:38Z

vispy/visuals/bar.py

+    orientation : {'h', 'v'}
+        Orientation of the histogram - 'h' is default


I find the current behaviour backwards: I would think h means that the bars are horizontal, but here it seems to refer to the orientation of the "baseline" of the plot. Is this typical?

What a philosophical question, on the one hand is it called horizontal if the bars got vertical up and are alined along the horizontal axis or is it called vertical.
I was thinking along which axis are the bars alined as opposed to the direction of the bars
I'm have no clue, I''ll use the vispy philosophical bend so its inline with other visuals

This is why I mentioned #2372. I assumed we were talking about the direction a single bar "grows". So if the bin dimension (width) is on the x-axis, then the bars grow on the y-axis and the plot is "vertical".

Bliss3d · 2022-10-04T11:59:47Z

Added individual colors per bar

brisvag

Still small suggestion left, but otherwise This is looking great for me! @djhoese anything to add?

brisvag · 2022-10-04T14:10:19Z

vispy/visuals/bar.py

+        if color_array is not None and color_array.shape[0] == height.shape[0] and color_array.shape[1] == 3:
+            MeshVisual.__init__(self, rr, tris, face_colors=np.repeat(color_array, 2, axis=0))
+        else:
+            MeshVisual.__init__(self, rr, tris, color=color)


Great! To get rid of this if clause (and get extra perks like string inputs and aplha values) I would use the ColorArray just like you did in the Candle PR.

@Bliss3d you think you can get this to work with a single color argument? I think it would be more intuitive, kinda like Markers does in these few lines:

vispy/vispy/visuals/markers.py

Lines 607 to 613 in f13d7da

edge_color = ColorArray(edge_color).rgba

if len(edge_color) == 1:

edge_color = edge_color[0]

face_color = ColorArray(face_color).rgba

if len(face_color) == 1:

face_color = face_color[0]

As you can see, you can use the ColorArray object from vispy.color, which automatically takes care of all the weird cases (a string like 'w', a (n, 3) rgb array, or an rgba, and so on). You can then simply check if it's a single color, and if so pass it to color, otherwise pass it to face_colors.

I'm guessing something like this

brisvag · 2022-10-05T12:00:07Z

vispy/visuals/bar.py

+        color_array = None
+
+        if isinstance(color, np.ndarray):
+            if len(color.shape) == 2:
+                if color.shape[1] == 3:
+                    if color.shape[0] > 1 and color.shape[0] != 0:
+                        color_array = np.repeat(color, 2, axis=0)
+                        color = None
+                else:
+                    raise ValueError('If you pass an numpy array as color it needs to be rgb i.e. 3 columns')
+        elif isinstance(color, ColorArray):
+            pass
+        elif isinstance(color, str):
+            pass
+        else:
+            raise ValueError("Color has to be either a vispy ColorArray, numpy array or a string for example: 'w'") 


Sorry, maybe I wasn't very clear: using the ColorArray is specifically to get rid of all this extra logic (and unnecessary restrictions like passing only rgb). You can do this:

Suggested change

color_array = None

if isinstance(color, np.ndarray):

if len(color.shape) == 2:

if color.shape[1] == 3:

if color.shape[0] > 1 and color.shape[0] != 0:

color_array = np.repeat(color, 2, axis=0)

color = None

else:

raise ValueError('If you pass an numpy array as color it needs to be rgb i.e. 3 columns')

elif isinstance(color, ColorArray):

pass

elif isinstance(color, str):

pass

else:

raise ValueError("Color has to be either a vispy ColorArray, numpy array or a string for example: 'w'")

color = ColorArray(color).rgba

if len(color_array):

color = color_array[0]

color_array = None

else:

color = None

and then later

MeshVisual.set_data(self, rr, tris, color=color, face_colors=color_array)

I'm not sure how this is supposed to work, color_array is not defiened in that context

sorry, wrote that in a rush. Fixed:

color = ColorArray(color).rgba if len(color): color = color[0] color_array = None else: color = None color_array = color

I modified it a bit, but I hope it works for you

brisvag · 2022-10-05T14:34:52Z

Yes, this looks good to me, thanks @Bliss3d! It wouyld be great to also have at least some basic tests before we merge. I don't think we need to do the test_image_approved route like Histogram does, which is a bit complicated, but maybe we can at least get some tests that check that nothing breaks when we try to draw it. @djhoese, any suggestions/preferences here?

djhoese · 2022-10-05T14:42:07Z

Yes @brisvag. Here's what I said early on that I think would be good for tests:

Otherwise, I forgot to mention, would it be possible to add a unit test for this new visual? Maybe something simple that shows that it is able to draw. You could even check the colors at certain points in the rendered result. For example, if the first bar is should fill the whole SceneCanvas then make sure that a color near the top of the image is the right color. And if the last bar is 50% of the canvas then make sure the colors are correct at the top and bottom half of the image.

Bliss3d · 2022-10-05T15:10:29Z

Well I have pretty much no clue how to run or create those test... :(

djhoese · 2022-10-05T19:19:53Z

Well I have pretty much no clue how to run or create those test... :(

Well there's no time like the present! VisPy has some basic documentation for writing tests (really glad past-Dave wrote this):

https://vispy.org/dev_guide/contributor_guide.html#adding-tests

In your case, you're writing tests for a Visual object so a new test module should be added here:

https://github.com/vispy/vispy/tree/main/vispy/visuals/tests

You'll find other tests in that directory to use as a starting point, but keep in mind that some of them do things like assert_image_approved (the histogram tests do this) where they compare against a known good image. The conversation @brisvag and I are having in the above comments is saying we don't want to use this. Creating, uploading, and maintaining these "known truth" images is annoying. Since your Visual is relatively simple as far as what it is drawing, it should be possible to get the result of the rendered canvas and then check a few specific values. Here's a portion of the test_mesh.py module where this test creates a Mesh object and then checks how many unique values there are in the Red channel (the result of c.render() is RGBA). This is very basic but should give you an idea that this rendered result is just a numpy array so if we know what colors to expect in certain parts of the result then we can just check those individual pixels as a "sanity check".

vispy/vispy/visuals/tests/test_mesh.py

Lines 194 to 213 in 859fcae

    
           @requires_pyopengl() 
        
           @requires_application() 
        
           def test_mesh_wireframe_filter(): 
        
               size = (45, 40) 
        
               with TestingCanvas(size=size, bgcolor="k") as c: 
        
                   v = c.central_widget.add_view(border_width=0) 
        
                   # Create visual 
        
                   mdata = create_sphere(20, 40, radius=20) 
        
                   mesh = scene.visuals.Mesh(meshdata=mdata, 
        
                                             shading=None, 
        
                                             color=(0.1, 0.3, 0.7, 0.9)) 
        
                   wireframe_filter = WireframeFilter(color='red') 
        
                   mesh.attach(wireframe_filter) 
        
                   v.add(mesh) 
        
                   from vispy.visuals.transforms import STTransform 
        
                   mesh.transform = STTransform(translate=(20, 20)) 
        
                   mesh.transforms.scene_transform = STTransform(scale=(1, 1, 0.01)) 
        
                   rendered_with_wf = c.render() 
        
                   assert np.unique(rendered_with_wf[..., 0]).size >= 50

Let me know if any of this isn't making sense.

Bliss3d · 2022-10-05T21:13:03Z

Will do, also I'll transfer the feedback to #2388 since there is quite a bit of overlap, but at the moment I need a change of pace from vertices and I still got the date axis labels in the works - and it is quite good and I need to flush it out before I dump all that time and date stuff out of my brain. I had this PR and #2388 lying around for mouths...

Bliss3d added 2 commits October 2, 2022 16:12

New Visual: Bar

324d4ba

flake8 corrected

847963e

Bliss3d mentioned this pull request Oct 2, 2022

I saw there was no option to plot bar graphs #2287

Closed

djhoese added type: enhancement component: visuals labels Oct 3, 2022

djhoese reviewed Oct 3, 2022

View reviewed changes

Added: Docstring, Lincense and Copyright

59a2791

djhoese reviewed Oct 3, 2022

View reviewed changes

Bliss3d added 2 commits October 3, 2022 23:17

Underline same number as characters

e4a4aab

Added: Vertical bar ploting

62b32aa

brisvag reviewed Oct 4, 2022

View reviewed changes

Apply and added suggested changes

8c5de6d

brisvag requested changes Oct 4, 2022

View reviewed changes

Bliss3d added 2 commits October 5, 2022 12:43

flake8, requested changes and swaped h and v

7a21064

Consolidated color and color_array parameters

f6c3fa6

brisvag reviewed Oct 5, 2022

View reviewed changes

changes to color logic

a4ca071


		base_faces = np.tile(base_faces, (height.shape[0], 1))

		faces = stack_n_x2*4 + base_faces

		orientation : {'h', 'v'}
		Orientation of the histogram - 'h' is default

	edge_color = ColorArray(edge_color).rgba
	if len(edge_color) == 1:
	edge_color = edge_color[0]

	face_color = ColorArray(face_color).rgba
	if len(face_color) == 1:
	face_color = face_color[0]

New Visual: Bar #2394

Are you sure you want to change the base?

New Visual: Bar #2394

Conversation

Bliss3d commented Oct 2, 2022

djhoese commented Oct 3, 2022

djhoese commented Oct 3, 2022

Choose a reason for hiding this comment

Bliss3d commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bliss3d commented Oct 3, 2022

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bliss3d commented Oct 4, 2022

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bliss3d Oct 5, 2022 • edited

Choose a reason for hiding this comment

brisvag Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brisvag commented Oct 5, 2022

djhoese commented Oct 5, 2022

Bliss3d commented Oct 5, 2022

djhoese commented Oct 5, 2022

Bliss3d commented Oct 5, 2022

Bliss3d Oct 5, 2022 •

edited

brisvag Oct 5, 2022 •

edited