NF - control the length of colorbar extensions #766
Conversation
| *extend* [ 'neither' | 'both' | 'min' | 'max' ] | ||
| If not 'neither', make pointed end(s) for out-of- | ||
| range values. These are set for a given colormap | ||
| using the colormap set_under and set_over methods. | ||
| *extendfrac* [ 'default' | None | 'auto' | length | lengths ] |
There was a problem hiding this comment.
In mpl, we use None as synonymous with default (typically from an rcparam). I would get rid of the use of 'neither' and stick with None. Also, in documentation, surround None with asterisks.
There was a problem hiding this comment.
I have used None as an alternative to 'default', 'neither' is used in the
'extend' argument which already existed and is nothing to do with my
code. Also, None is not surrounded by asterisks in any of the other
docstrings in colorbar.py, I thought it best to be consistent? Do I need to
change anything here?
On 14 March 2012 21:29, Benjamin Root <
reply@reply.github.com
wrote:
*extend* [ 'neither' | 'both' | 'min' | 'max' ] If not 'neither', make pointed end(s) for out-of- range values. These are set for a given colormap using the colormap set_under and set_over methods.
- extendfrac [ 'default' | None | 'auto' | length | lengths ]
In mpl, we use None as synonymous with default (typically from an
rcparam). I would get rid of the use of 'neither' and stick with None.
Also, in documentation, surround None with asterisks.
Reply to this email directly or view it on GitHub:
https://github.com/matplotlib/matplotlib/pull/766/files#r560111
There was a problem hiding this comment.
I have used None as an alternative to 'default', 'neither' is used in the
'extend' argument which already existed and is nothing to do with my
code. Also, None is not surrounded by asterisks in any of the other
docstrings in colorbar.py, I thought it best to be consistent? Do I need
to
change anything here?
I don't think we should have two argument values that have identical
affects. Standard practice in mpl is to have None as default. Eventually,
this could be an rcparam and the practice there is to have None mean "get
the rcparam value".
As for the asterisks, doc styles have not been enforced, until now. I am
working on updating the existing docs, and it makes it easier on me if I
make sure all new code follow this.
There was a problem hiding this comment.
I agree that it would be cleaner to eliminate "default" in favor of None in this case. @ajdawson is correct that "neither" must be left alone as an option to the original extend; I think that @WeatherGod actually meant "default".
There was a problem hiding this comment.
@WeatherGod: This is really off-topic, but is there any chance that Matplotlib will migrate to Numpy's docstring standards?
There was a problem hiding this comment.
Correct, I meant "default', sorry for the confusion.
Tony, I have to read through their standard and see how comprehensive they are. Maybe mpl's standard might have to be a superset of it? I do have some minor issues with how they do things, but any standard is better than none!
|
Can you add an example and a unit test? |
|
With regards to a unit test, I think need some advice. With the revision of master that this PR is branched from, many tests are failing, I think due to problems comparing images. This is fixed in the current master. I am not able to write a functioning test in this branch without these fixes. It says in the developer's guide that one should not merge the current master into a feature branch to keep history as clean as possible. What would be the best thing to do in this scenario? |
|
You will need to rebase your branch against the current master anyway. When you rebase, you will need to address any merge conflicts and then push that result back up to your github repo. The changes should then be reflected here. You can then made additional commits for unit tests as needed. |
|
@ajdawson: Agreed about not merging master into the feature branch. However, rebasing the feature branch on master is fine -- it basically replacing your branch's history with a new history built on top of the new master. I find this quite useful to revive out of date branches. |
|
I guess it is safe to ignore the usual warnings about not rebasing stuff that has been pushed to a public repository in this instance and force git to push the changes? |
|
Yes. Force pushing is only usually a problem if you know other people have based a branch off of yours. So that definitely applies to the |
|
I've written a new test module for testing colorbars with extensions. I've also added to the existing colorbar example in the API examples, it seemed like the most appropriate place to put an example of this feature. Let me know if there is something I have overlooked. |
| # If frac is a sequence contaning None then NaN may | ||
| # be encountered. This is an error. | ||
| if np.isnan(extendlength).any(): | ||
| raise ValueError |
There was a problem hiding this comment.
Stylistically this should be a ValueError instance. i.e. ValueError().
|
I think this is a really well put together PR and, IMHO, is at a stage where this is good to be merged in. Thanks for all your work @ajdawson. |
|
+1 from me. |
|
Agreed. The only thing missing that I can see is an addition to the "matplotlib/doc/users/whats_new.rst" and "matplotlib/doc/api/api_changes.rst". At least one of the two, if not both. |
|
I just followed the style of the other entries in the whats_new.rst and api_changes.rst files. I hope these are appropriate. |
|
Your edits look good, just need a rebase |
Added ability to control length of colorbar extensions using a new keyword argument "extendfrac". Tests and examples are also included.
|
I rebased onto master and squashed my commits into a single commit. |
|
Looks good. Thanks for your work. Merging... |
NF - control the length of colorbar extensions
Added the ability to control the length of colorbar extensions using a new keyword argument "extendfrac" to matplotlib.pyplot.colorbar.