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

Lightsource shade method parameters for color range definition #2650

Merged
merged 7 commits into from Dec 18, 2013

Conversation

syngron
Copy link

@syngron syngron commented Dec 5, 2013

Added vmin and vmax arguments to LigthSource shade method which, if set, changes the lower and upper border of the colorspace. Compare with method contourf where these parameters are available. Basically they are needed if one has data in a smaller range than the colospace one wants to use. Normally the shade method just takes data.min() and data.max() as the range, by defining vmin and vmax thos get overwritten.

changes the lower and upper border of the colorspace. Compare with
method contourf where these parameters are available.
@pelson
Copy link
Member

pelson commented Dec 5, 2013

How about accepting a norm argument instead and being consistent with most of the other routines in mpl?

Would you mind adding a test for this, I suspect there are 0 tests for the entire function...

Thanks @syngron.

@syngron
Copy link
Author

syngron commented Dec 5, 2013

The error in travis now comes from the test "ERROR: matplotlib.tests.test_contour.test_contour_manual_labels.test" which is IMHO not related to my added test.

@tacaswell
Copy link
Member

Please don't merge master into PR branches, it makes merging this branch back into master messy.

Can you use git push --force to roll your head back by one commit? If you want to include changes from master in your branch it is better to use rebase.

@syngron
Copy link
Author

syngron commented Dec 5, 2013

Ok, I because this other error came up I thought that I have to merge them before new commits.

"git push --force" gives only "Everything up-to-date". How can I proceed to fix this?

@@ -205,6 +205,35 @@ def gray_from_float_rgba():
assert_raises(ValueError, gray_from_float_rgba)


def test_light_source_shading_color_range():
Copy link
Member

Choose a reason for hiding this comment

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

What is this testing? It doesn't actually assert anything as far as I can see.
Perhaps you could construct a LightSource and call its shade method with a small array (like a (3, 4)) and compare that with an expected result (using numpy.testing.assert_array_equal or perhaps numpy.testing.assert_array_almost_equal etc.).

@pelson
Copy link
Member

pelson commented Dec 5, 2013

@syngron - thanks for putting the norm as an argument to LightShade - I think this looks much better now and is consistent with most other color/scalable type objects in matplotlib. I've made a couple of comments, but I think this is looking really promising, so thank you for persevering!

@tacaswell
Copy link
Member

Sorry, should have been clearer, you need to roll back your local branch by one commit first (I normally do this using gitk, I think git reset --hard HEAD~1 will do it from the command line) and then do the git push --force

- basic example and assert statement in LightSource test
@syngron
Copy link
Author

syngron commented Dec 6, 2013

Ok thanks for the hints 👍 Sorry for the confusions but this is my first pull request.

I admit that the test case was not a real test before, I wanted to change that later, now I did it. I also included the norm simplification in the shade method.

@pelson
Copy link
Member

pelson commented Dec 6, 2013

This is looking good to me, thanks for your hard work @syngron.
I'm going to merge this on Monday to give others a chance to take a look before hand.

Thanks!

@tacaswell
Copy link
Member

@pelson You happy with merging this?

@pelson
Copy link
Member

pelson commented Dec 18, 2013

Thanks for the nudge @tacaswell.

pelson added a commit that referenced this pull request Dec 18, 2013
Lightsource shade method parameters for color range definition
@pelson pelson merged commit 1a9fa9e into matplotlib:master Dec 18, 2013
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.

None yet

4 participants