Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
getter for ticks for colorbar #6170
Conversation
mdboom
added the
needs_review
label
Mar 16, 2016
|
Can you add a simple test for this? Is this consistent with similar method on the x and y axis? Does this do the mapping to data space? |
tacaswell
added this to the
2.1 (next point release)
milestone
Mar 17, 2016
|
I've checked and it does work properly with both a horizontal and vertical axis for a colorbar. In relation to testing, what would be consider a good test? is simply comparing the ticks that are set and the ticks that are return by the method enough? Also, still wondering if having ticks returned in a list is acceptable. |
tacaswell
commented on an outdated diff
Mar 21, 2016
| @@ -396,6 +396,14 @@ def set_ticks(self, ticks, update_ticks=True): | ||
| self.update_ticks() | ||
| self.stale = True | ||
| + def get_ticks(self): | ||
| + """ | ||
| + gets the tick locations that are in the locator | ||
| + :return: an array of all ticks in the locator | ||
| + """ | ||
| + | ||
| + return self.locator.tick_values(0, 0) |
|
|
|
By consistent, I mean what would you use to get the ticks are for the x or y axis. A good test would be to construct an example where you know what the tick should be and then check that they are in fact what you expect. I think this is going to have to be a bit more involved and possible involve a refactor of |
tacaswell
added needs_revision and removed needs_review
labels
Mar 21, 2016
|
I've created the necessary implementation of xticks and yticks in correlation to the behaviour that axis class does. I'm creating the tests right now for my implementation . Also, refactoring _ticker might not be necessary considering that the 2 methods I created are the same as axis's ticker getters behaviour. |
|
@yongzhez I don't think those two methods are useful for Colorbar. It looks like #6216 is closer to doing the right thing, but still needs work. Notice that a Colorbar has ticks only on one axis; and that the tick locations as returned by your methods are not in data coordinates, so they don't address #5792. |
|
I see, I used your suggestion to basically add a _tick_data_values field. I also added a test to correspond with this to test for the 2 returned list for a horizontal and vertical colorbar. I chose to go the path of a getter simply for consistency sake, but I can change when required. |
|
I've added the case that the user does not set their own ticks and updated the tests to correspond. |
|
I'm sorry, but I think your approach is still not correct. We don't want to recalculate the data tick values, we want to save them at the point where they are already being calculated. And it looks to me like the values you are recalculating or grabbing are not the data values at all. |
|
I apologize @efiring , I have noted that update_ticks() already calls _ticker() so I can just assign the ticks there. I also now return the data as a list of all the ticks. Is that sufficient? |
|
@yongzhez I think you missed a critical point: the "ticks" returned by |
|
@efiring it's pretty late where I live so I wasn't able to contact the other team, I think I understand what you want me to do and I pushed a fix. |
efiring
commented on an outdated diff
Mar 28, 2016
| @@ -592,6 +597,7 @@ def _ticker(self): | ||
| formatter.set_data_interval(*intv) | ||
| b = np.array(locator()) | ||
| + self._tick_data_values = b |
efiring
Owner
|
efiring
commented on an outdated diff
Mar 28, 2016
| @@ -298,6 +298,26 @@ def test_colorbar_ticks(): | ||
| assert len(cbar.ax.xaxis.get_ticklocs()) == len(clevs) | ||
| +@cleanup | ||
| +def test_colorbar_get_ticks(): | ||
| + # test feature for #5792 | ||
| + plt.figure() | ||
| + data = np.arange(1200).reshape(30, 40) | ||
| + | ||
| + plt.subplot() | ||
| + plt.contourf(data) |
efiring
Owner
|
efiring
commented on an outdated diff
Mar 28, 2016
| @@ -298,6 +298,26 @@ def test_colorbar_ticks(): | ||
| assert len(cbar.ax.xaxis.get_ticklocs()) == len(clevs) | ||
| +@cleanup | ||
| +def test_colorbar_get_ticks(): | ||
| + # test feature for #5792 | ||
| + plt.figure() | ||
| + data = np.arange(1200).reshape(30, 40) | ||
| + | ||
| + plt.subplot() | ||
| + plt.contourf(data) | ||
| + | ||
| + userTicks = plt.colorbar(use_gridspec=True, ticks=[0, 600, 1200]) |
efiring
Owner
|
efiring
commented on an outdated diff
Mar 28, 2016
| + # test feature for #5792 | ||
| + plt.figure() | ||
| + data = np.arange(1200).reshape(30, 40) | ||
| + | ||
| + plt.subplot() | ||
| + plt.contourf(data) | ||
| + | ||
| + userTicks = plt.colorbar(use_gridspec=True, ticks=[0, 600, 1200]) | ||
| + assert userTicks.get_ticks().tolist() == [0, 600, 1200] | ||
| + | ||
| + defTicks = plt.colorbar(use_gridspec=True, orientation='horizontal') | ||
| + assert defTicks.get_ticks().tolist() == [0.0, 150.0, 300.0, 450.0, 600.0, | ||
| + 750.0, 900.0, 1050.0, 1200.0] | ||
| + | ||
| + userTicks.set_ticks([600, 700, 800]) | ||
| + assert userTicks.get_ticks().tolist() == [600, 700, 800] |
efiring
Owner
|
efiring
commented on an outdated diff
Mar 28, 2016
| @@ -298,6 +298,26 @@ def test_colorbar_ticks(): | ||
| assert len(cbar.ax.xaxis.get_ticklocs()) == len(clevs) | ||
| +@cleanup | ||
| +def test_colorbar_get_ticks(): | ||
| + # test feature for #5792 | ||
| + plt.figure() | ||
| + data = np.arange(1200).reshape(30, 40) | ||
| + | ||
| + plt.subplot() | ||
| + plt.contourf(data) | ||
| + | ||
| + userTicks = plt.colorbar(use_gridspec=True, ticks=[0, 600, 1200]) | ||
| + assert userTicks.get_ticks().tolist() == [0, 600, 1200] | ||
| + | ||
| + defTicks = plt.colorbar(use_gridspec=True, orientation='horizontal') | ||
| + assert defTicks.get_ticks().tolist() == [0.0, 150.0, 300.0, 450.0, 600.0, |
efiring
Owner
|
|
@efiring I made the changes. |
efiring
merged commit 5d0ca1d
into matplotlib:master
Mar 31, 2016
efiring
removed the
needs_revision
label
Mar 31, 2016
|
Thank you, @yongzhez! |
yongzhez commentedMar 16, 2016
This is my proposed inclusion for #5792. I do note that it hands backs the ticks in a list instead of an array, is that a problem by any chance?