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

Update custom marker as scale_ctm expects floats #782

Merged
merged 4 commits into from
Apr 22, 2021

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Apr 16, 2021

Needed to close enthought/chaco#232

Down in agg the method that gets called wants double inputs. If an int gets passed it things break. Changing the inputs to floats seems to resolve the problem.

@rahulporuri
Copy link
Contributor

I don't see any unittests for the CustomMarker code so let's take advantage of this opportunity to add some.

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM. Ref

void scale_ctm(double sx, double sy);
.

I'd still like it if we could add a few tests.

@aaronayres35
Copy link
Contributor Author

I'd still like it if we could add a few tests.

👍 I've added 2 regression tests

@aaronayres35
Copy link
Contributor Author

CI is failing with

======================================================================
ERROR: test_get_compiled_path (enable.tests.test_markers.TestCustomMarker)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/enable/enable/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/enable/tests/test_markers.py", line 50, in test_get_compiled_path
    marker.get_compiled_path(np.int64(0))
  File "/Users/runner/work/enable/enable/.edm/envs/enable-test-3.6-wx/lib/python3.6/site-packages/enable/markers.py", line 424, in get_compiled_path
    newpath.scale_ctm(float(size), float(size))
AttributeError: 'kiva.quartz.ABCGI.CGMutablePath' object has no attribute 'scale_ctm'

----------------------------------------------------------------------

perhaps this test just needs to be skipped on some combination of quartz / macos / wx? I am marking this as a WIP until I get a chance to look into this more

@aaronayres35 aaronayres35 changed the title Update custom marker as scale_ctm expects floats [WIP] Update custom marker as scale_ctm expects floats Apr 19, 2021
@aaronayres35 aaronayres35 changed the title [WIP] Update custom marker as scale_ctm expects floats Update custom marker as scale_ctm expects floats Apr 21, 2021
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Still LGTM

Comment on lines +14 to +17
from enable.compiled_path import CompiledPath
from enable.kiva_graphics_context import GraphicsContext
from enable.markers import CustomMarker
from enable.tests._testing import skip_if_not_qt
Copy link
Contributor

Choose a reason for hiding this comment

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

not an actionable comment - i was wondering if the enable testsuite should always import from enable.api instead of importing from the specific modules. Note that this is a more general question for ETS/package development.

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 notice some of the test files do but others don't. I think it probably makes sense to use api everywhere in tests

@aaronayres35 aaronayres35 merged commit b93129e into master Apr 22, 2021
@aaronayres35 aaronayres35 deleted the fix-custom-marker-get_compiled_path branch April 22, 2021 12:13
@jwiggins
Copy link
Member

perhaps this test just needs to be skipped on some combination of quartz / macos / wx? I am marking this as a WIP until I get a chance to look into this more

This behavior of CompiledPath is only possible with kiva.agg due to a design defect (see #716)

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.

Colormapped scatterplot with custom marker render fails
3 participants