-
Notifications
You must be signed in to change notification settings - Fork 29
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
Include tests for image_type #93
base: master
Are you sure you want to change the base?
Conversation
@JohnnyTeutonic now you should add a test case that raises this error. =) |
weird build fail? in 'setup.py'. very confused, as that environment variable is present in all your setup.py scripts |
tests/test_viz.py
Outdated
def test_multiple_images(): | ||
TEST_IM_1 = np.random.randn(30, 30) | ||
TEST_IM_2 = np.random.randn(30, 30) | ||
show_multiple_images(TEST_IM_1, TEST_IM_2, image_type='colour') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to add a test that raises the LookupError. And actually I've never heard of such an error, I suggest ValueError instead!
Yes I've actually spent all of today fixing this in various packages, that's why I was even looking at the gala repo. =) setuptools v38 introduced a breaking change that install requirements could only be ordered containers (lists, tuples) and not unordered ones (sets) so that the order of package installation would be fixed. If you rebase on master you should see the error disappear. |
Ah that makes a hell of a lot of sense. I'll try and see if I can figure
out how to rebase.
What do you mean? I input the 'colour' parameter and that raised the
LookupError?
…On 12 Feb 2018 6:07 pm, "Juan Nunez-Iglesias" ***@***.***> wrote:
Yes I've actually spent all of today fixing this in various packages,
that's why I was even looking at the gala repo. =) setuptools v38
introduced a breaking change that install requirements could only be
ordered containers (lists, tuples) and not unordered ones (sets) so that
the order of package installation would be fixed. If you rebase on master
you should see the error disappear.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#93 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AOIZLS145ORcYDlJ-siulSsQMGorV_m-ks5tT-NDgaJpZM4OHY0R>
.
|
Ah I should rename my test function! That's why you think i haven't tested
it.
…On 12 Feb 2018 6:25 pm, "Jonathan Reich" ***@***.***> wrote:
Ah that makes a hell of a lot of sense. I'll try and see if I can figure
out how to rebase.
What do you mean? I input the 'colour' parameter and that raised the
LookupError?
On 12 Feb 2018 6:07 pm, "Juan Nunez-Iglesias" ***@***.***>
wrote:
> Yes I've actually spent all of today fixing this in various packages,
> that's why I was even looking at the gala repo. =) setuptools v38
> introduced a breaking change that install requirements could only be
> ordered containers (lists, tuples) and not unordered ones (sets) so that
> the order of package installation would be fixed. If you rebase on master
> you should see the error disappear.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#93 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AOIZLS145ORcYDlJ-siulSsQMGorV_m-ks5tT-NDgaJpZM4OHY0R>
> .
>
|
Put in a test for the function 'viz.show_multiple_images' to ensure that the 'image_type' parameter has sane output