MAINT: fix marker size type mismatch error by homosapien-lcy · Pull Request #859 · enthought/chaco

homosapien-lcy · 2023-03-13T04:18:39Z

The ScatterPlot1D specifies marker_size = Float(4.0) while the downstream render_markers function is supposed to handle both single numeric data and arrays. This PR changes this.

Address #846

homosapien-lcy · 2023-03-13T08:26:44Z

close issue #846

dpinte · 2023-03-23T13:21:46Z

-    marker_size = Float(4.0)
+    marker_size = Union(Float, Int,
+                        Array(Float), Array(Int),
+                        default_value=None)


Why changing the defaut to None? The 4.0 value was a good default. By changing the default, you now have to update the _render method.

The upstream issue was related to the fact that only floats were supported. Using a Union type, you now allow other data type, which is solving the problem. Adding a tests that reproduces the uptsream problem would be good

I somehow remembered that using 4.0 as default will raise an error, but after retest, works fine... Will use 4.0 instead.

@homosapien-lcy adding a new manual example is not adding a test!
As discussed, tests are based on the unittest.TestCase in order too be picked up by the CI and shoud be added here: https://github.com/enthought/chaco/blob/main/chaco/plots/tests/test_scatterplot_1d.py

mdickinson · 2023-03-24T08:21:46Z


    # The pixel size of the marker, not including the thickness of the outline.
-    marker_size = Float(4.0)
+    marker_size = Union(Float, Int,


The Float trait should already accept integers, so I'm not sure that it adds much to accept Int here, especially given that the marker size is conceptually a float here.

Yeah, I found that Float trait can accomodate int as well, just cahnged.

mdickinson · 2023-03-24T08:24:54Z

    # The pixel size of the marker, not including the thickness of the outline.
-    marker_size = Float(4.0)
+    marker_size = Union(Float, Int,
+                        Array(Float), Array(Int),


This isn't how the Array trait works - you need to supply a dtype as the argument, not a trait type. As it stands, this would accept an array with any dtype.

Got it, thanks!

dpinte · 2023-03-24T09:10:44Z

@homosapien-lcy as next step, we discussed:

add a test case reproducing the upstream issue, that should surface the problem that @mdickinson has highlighted with the way you use the Array trait
fix the current version of the code and make sure the test pass
add tests cases for the various other use case you cover in the new Union (and verify that as Mark suggests, it's not needed to add the Int trait in the Union)

dpinte

@homosapien-lcy some comments to address on the test.

dpinte · 2023-03-30T07:55:26Z

    # The pixel size of the marker, not including the thickness of the outline.
-    marker_size = Float(4.0)
+    marker_size = Union(Float,
+                        Array(float),


make the keyword argument explicit with dtype=float

dpinte · 2023-03-30T08:05:14Z

+
+
+class Scatter1DTestCase(unittest.TestCase):
+    # test the default value 4.0


that comment line can be removed in favour of a clear method name or moved as a docstring to the test method.

dpinte · 2023-03-30T08:05:42Z

+
+class Scatter1DTestCase(unittest.TestCase):
+    # test the default value 4.0
+    def test_default(self):


I would suggests test_default_marker_size rather than test_default

dpinte · 2023-03-30T08:06:50Z

+            orientation="v",
+            marker="plus",
+            alignment="left"
+        )


nothing is really being tested here. What do you expect to test? Verify that something is generated without exception? ...

that is applicable to all the tests.

Yes, these tests are limited to check for traits mismatch exceptions. Is there other stuffs we want to test?

If this is a test of the default marker size, you should somewhere have a line like self.assertEqual(..., 4.0)

dpinte · 2023-03-30T08:07:03Z

+        )
+
+    # test integer marker size
+    def test_int(self):


same comment on the method name and the removal of the comment

Thanks for the comments, updates made

corranwebster

Not really sure what the point of this PR is. The 1D scatterplot doesn't currently support variable size markers (or variable color or shape, for that matter). It could, potentially, but it doesn't right now, and this PR is not correct in its approach. Chaco is a dynamoc plotting library and all data needs to be passed via appropriate objects that manage changes to the data. For example, the example usage will fail if the using code changes the size of the data array holding the values.

To be fair, it looks like there is an error in the corresponding Trait declarations for the 2D scatterplot: the following is not correct either (edit: as is noted in the "TODO")

chaco/chaco/plots/scatterplot.py

Lines 228 to 231 in f32e01c

    
           # The pixel size of the markers, not including the thickness of the outline. 
        
           # Default value is 4.0. 
        
           # TODO: for consistency, there should be a size data source and a mapper 
        
           marker_size = Union(Float, Array, requires_redraw=True)

A correct implementation would use a data source for the sizes (and possibly a mapper as well), the way that the colormapped scatterplot does for colors:

chaco/chaco/plots/colormapped_scatterplot.py

Lines 65 to 66 in f32e01c

    
           #: Source for color data. 
        
           color_data = Instance(ArrayDataSource)

It would also have to add support for finding the data source in the Plot object code, eg. somewhere in here:

chaco/chaco/plot.py

Lines 1270 to 1296 in f32e01c

    
           if plot_type in ("scatter_1d", "line_scatter_1d", "jitterplot"): 
        
               # simple 1d positional plots with no associated value 
        
               for source in data: 
        
                   index = self._get_or_create_datasource(source) 
        
                   index_range.add(index) 
        
                   if scale == "linear": 
        
                       imap = LinearMapper( 
        
                           range=index_range, 
        
                           stretch_data=index_mapper.stretch_data, 
        
                       ) 
        
                   else: 
        
                       imap = LogMapper( 
        
                           range=index_range, 
        
                           stretch_data=index_mapper.stretch_data, 
        
                       ) 
        
                   cls = self.renderer_map[plot_type] 
        
                   plot = cls( 
        
                       index=index, 
        
                       index_mapper=imap, 
        
                       orientation=orientation, 
        
                       direction=direction, 
        
                       **styles 
        
                   ) 
        
                   plots.append(plot) 
        
                   self.add(plot)

corranwebster · 2023-04-06T07:25:57Z

+    marker_size = Union(Float,
+                        Array(dtype=float),
+                        default_value=4.0)


This is absolutely not correct - Chaco data never comes from hard-coded arrays like this. Most likely this should be a Float or an Instance(AbstractDataSource).

corranwebster · 2023-04-06T07:30:13Z

+        orientation="v",
+        marker="plus",
+        alignment="left",
+        marker_size=randint(1,5, numpts)


This is not the correct usage and should fail. You would need to specify a data source for the size values which holds the sizes.

corranwebster · 2023-04-06T07:31:49Z

+            orientation="v",
+            marker="plus",
+            alignment="left"
+        )


If this is a test of the default marker size, you should somewhere have a line like self.assertEqual(..., 4.0)

corranwebster · 2023-04-06T07:51:54Z

+# Thanks for using Enthought open source!
+
+"""
+Unit tests for scatter_1d function


There are already tests for the 1D scatterplot here: https://github.com/enthought/chaco/blob/main/chaco/plots/tests/test_scatterplot_1d.py

homosapien-lcy · 2023-04-20T00:36:27Z

With Corran's comments, after discussing with Didrik, we think:

1 the "error" that the user reported in #846 is actually the expected behavior of the test, thus there is no need to change the scatterplot class to enable different types of inputs
2 the confusion comes from the fact that in chaco/examples/demo/basic/scatter_1d, a line "# randint(1,5, numpts)," is commented out in line 78, suggesting the users that the input may be an array of integers.

With the above observations, we decided to simply delete the commented out line mentioned in 2 to avoid confusion as the solution to the issue closes #846

MAINT: fix marker size type mismatch error

95afa2e

homosapien-lcy mentioned this pull request Mar 13, 2023

ScatterPlot example currently shows marker_size takes integer arrays #846

Open

Chengyu Liu added 4 commits March 13, 2023 13:35

add marker size check

3e71256

move default marker size to scatter plot 1d

64865c9

FORMAT: use Union to have more specific traits for marker size

ea1d2eb

None as default

8142ffc

Chengyu Liu added 2 commits March 13, 2023 17:28

fix default name

d90fe94

fix line too long

eabdfeb

homosapien-lcy requested a review from corranwebster March 13, 2023 08:43

dpinte requested changes Mar 23, 2023

View reviewed changes

Chengyu Liu added 2 commits March 24, 2023 17:09

use 4 as default value

0c178d2

add new marker_size test

cf34625

mdickinson reviewed Mar 24, 2023

View reviewed changes

Chengyu Liu added 6 commits March 30, 2023 15:08

add tests for scatterplot inputs

fe2695d

git push -u origin fix_scatter_plot_type_error

fecfa0d

FORMAT: fix small formatting

e03dbcd

remove whitespace

579f4a0

add copyright and image source

a9f0455

move copyright to another branch

0bd3be7

dpinte requested changes Mar 30, 2023

View reviewed changes

FORMAT: 1 method name reformatting 2 set data type in the trait array

94c3b41

corranwebster suggested changes Apr 6, 2023

View reviewed changes

Chengyu Liu added 2 commits April 20, 2023 09:25

MAINT: remove confusing line from the scatter_id demo

522f272

delete unneeded tests

4f3c87b

corranwebster approved these changes May 4, 2023

View reviewed changes

Merge branch 'main' into fix_scatter_plot_type_error

56ac53d

dpinte approved these changes May 16, 2023

View reviewed changes

dpinte merged commit fa42199 into main May 16, 2023

dpinte deleted the fix_scatter_plot_type_error branch May 16, 2023 12:40



		class Scatter1DTestCase(unittest.TestCase):
		# test the default value 4.0

	# The pixel size of the markers, not including the thickness of the outline.
	# Default value is 4.0.
	# TODO: for consistency, there should be a size data source and a mapper
	marker_size = Union(Float, Array, requires_redraw=True)

	#: Source for color data.
	color_data = Instance(ArrayDataSource)

	if plot_type in ("scatter_1d", "line_scatter_1d", "jitterplot"):
	# simple 1d positional plots with no associated value
	for source in data:
	index = self._get_or_create_datasource(source)
	index_range.add(index)

	if scale == "linear":
	imap = LinearMapper(
	range=index_range,
	stretch_data=index_mapper.stretch_data,
	)
	else:
	imap = LogMapper(
	range=index_range,
	stretch_data=index_mapper.stretch_data,
	)

	cls = self.renderer_map[plot_type]
	plot = cls(
	index=index,
	index_mapper=imap,
	orientation=orientation,
	direction=direction,
	**styles
	)
	plots.append(plot)
	self.add(plot)

Conversation

homosapien-lcy commented Mar 13, 2023 • edited by dpinte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

homosapien-lcy commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpinte commented Mar 24, 2023 • edited by homosapien-lcy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpinte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranwebster Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranwebster left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranwebster Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corranwebster Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

homosapien-lcy commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

homosapien-lcy commented Mar 13, 2023 •

edited by dpinte

Loading

dpinte commented Mar 24, 2023 •

edited by homosapien-lcy

Loading

corranwebster Apr 6, 2023 •

edited

Loading

corranwebster left a comment •

edited

Loading

corranwebster Apr 6, 2023 •

edited

Loading

corranwebster Apr 6, 2023 •

edited

Loading