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.

Sign up for GitHub

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

MAINT: fix marker size type mismatch error #859

Merged
merged 19 commits into from
May 16, 2023
Merged

MAINT: fix marker size type mismatch error #859

Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
95afa2e
MAINT: fix marker size type mismatch error
Mar 13, 2023
3e71256
add marker size check
Mar 13, 2023
64865c9
move default marker size to scatter plot 1d
Mar 13, 2023
ea1d2eb
FORMAT: use Union to have more specific traits for marker size
Mar 13, 2023
8142ffc
None as default
Mar 13, 2023
d90fe94
fix default name
Mar 13, 2023
eabdfeb
fix line too long
Mar 13, 2023
0c178d2
use 4 as default value
Mar 24, 2023
cf34625
add new marker_size test
Mar 24, 2023
fe2695d
add tests for scatterplot inputs
Mar 30, 2023
fecfa0d
git push -u origin fix_scatter_plot_type_error
Mar 30, 2023
e03dbcd
FORMAT: fix small formatting
Mar 30, 2023
579f4a0
remove whitespace
Mar 30, 2023
a9f0455
add copyright and image source
Mar 30, 2023
0bd3be7
move copyright to another branch
Mar 30, 2023
94c3b41
FORMAT: 1 method name reformatting 2 set data type in the trait array
Mar 31, 2023
522f272
MAINT: remove confusing line from the scatter_id demo
Apr 20, 2023
4f3c87b
delete unneeded tests
Apr 20, 2023
56ac53d
Merge branch 'main' into fix_scatter_plot_type_error
corranwebster May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions chaco/plots/scatterplot_1d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

# Enthought library imports
from enable.api import black_color_trait, ColorTrait, MarkerTrait
from traits.api import Any, Bool, Callable, Enum, Float, Str
from traits.api import Any, Array, Bool, Callable, Enum, Float, Int, Str, Union

# local imports
from chaco.base_1d_plot import Base1DPlot
Expand All @@ -32,7 +32,9 @@ class ScatterPlot1D(Base1DPlot):
marker = MarkerTrait

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Array(Float), Array(Int),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

default_value=None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


# The CompiledPath to use if **marker** is set to "custom". This attribute
# must be a compiled path for the Kiva context onto which this plot will
Expand Down Expand Up @@ -111,6 +113,10 @@ def _draw_plot(self, gc, view_bounds=None, mode="normal"):
self._render(gc, pts)

def _render(self, gc, pts):
# check for None marker size and give a default value
if self.marker_size is None:
self.marker_size = 4.0

with gc:
gc.clip_to_rect(self.x, self.y, self.width, self.height)
if not self.index:
Expand Down