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

Specify size_or_ary contains ints #788

Merged
merged 1 commit into from
Jun 21, 2021
Merged

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Jun 15, 2021

part of #785
As mentioned in the issue, we can resolve many test suite errors with celiagg by enforcing these be ints. Note that scale itself I think can be a float (e.g. the scale_ctm method expects floats as arguments), however, size_or_ary gets used as a _width and _height and is assumed to contain integers.

Note, with this change I now see 2 failures in the test suite: EDIT: I only see the one error mentioned on the issue:

======================================================================
FAIL: test_draw_border_simple (chaco.tests.test_border.DrawBorderTestCase)
Borders should have the correct height and width.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aayres/Desktop/chaco/chaco/tests/test_border.py", line 52, in test_draw_border_simple
    self.assertRavelEqual(actual, desired)
  File "/Users/aayres/Desktop/chaco/chaco/tests/test_border.py", line 29, in assertRavelEqual
    alltrue(ravel(x) == ravel(y)), "\n%s\n !=\n%s" % (x, y)
AssertionError: False is not true : 
[[  0   0   0   0   0   0]
 [  0   0   0   0   0   0]
 [  0   0 255 255   0   0]
 [  0   0 255 255   0   0]
 [  0   0   0   0   0   0]
 [  0   0   0   0   0   0]]
 !=
[[255 255 255 255 255 255]
 [255   0   0   0   0 255]
 [255   0 255 255   0 255]
 [255   0 255 255   0 255]
 [255   0   0   0   0 255]
 [255 255 255 255 255 255]]

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. I verified that kiva GraphicsContext expects heigth, width of image to be ints, not floats.

@@ -34,8 +34,8 @@ def __init__(self, size_or_ary, *args, **kw):
scale = kw.pop("dpi", 72.0) / 72.0
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can assume that size_or_ary is a tuple of ints, we can make the following change instead (IIUC the code correctly) but we can be defensive and go with the changes you propose.

Suggested change
scale = kw.pop("dpi", 72.0) / 72.0
scale = kw.pop("dpi", 72.0) // 72.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's alright with you I think I will leave the current approach, as you mention, to be defensive. The reasoning being that I think scale_ctm can accept floats and size_or_ary is what we specifically want to have integer entries.
I would guess in practice scale will typically be an integer but I am not certain that would always be the case

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.

2 participants