Skip to content

Allow tuples of 4 floats as color rcparams. #9038

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

Merged
merged 3 commits into from
Aug 22, 2017

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Aug 15, 2017

Fixes #9030 (comment).
(Although I still think (as argued in many other places) that the proper solution is to use Python files, or at least a full Python expression evaluator, as rcfile.)

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@afvincent
Copy link
Contributor

Huh... validate_color is not tested in test_rcparams.py?

@afvincent
Copy link
Contributor

@anntzer Would it be worth to add this kind of snippet

# One would need to also import validate_color beforehand

        {'validator': validate_color,
         'success': (('None', 'none'),
                     ('none', 'none'),
                     ('AABBCC', '#AABBCC'),  # RGB hex code
                     ('AABBCC00', '#AABBCC00'),  # RGBA hex code
                     ('tab:blue', 'tab:blue'),  # named color
                     ('C0', 'C0'),  # color from cycle
                     ('(0, 1, 0)', [0.0, 1.0, 0.0]),  # RGB tuple
                     ((0, 1, 0), (0, 1, 0)),  # non-string version
                     ('(0, 1, 0, 1)', [0.0, 1.0, 0.0, 1.0]),  # RGBA tuple
                     ((0, 1, 0, 1), (0, 1, 0, 1)),  # non-string version
                     ('(0, 1, "0.5")', [0.0, 1.0, 0.5]),  # unusual but valid
                     
                    ),
         'fail': (('tab:veryblue', ValueError),  # invalid name
                  ('C123', ValueError),  # invalid RGB(A) code and cycle index
                  ('(0, 1)', ValueError),  # tuple with length < 3
                  ('(0, 1, 0, 1, 0)', ValueError),  # tuple with length > 4
                  ('(0, 1, none)', ValueError),  # cannot cast none to float
                 ),

to generate_validator_testcases in test_rcparams.py?

FWIW, currently (on Matplotlib 2.0.2) validate_color((0, 1)) or validate_color((0, 1, 0, 1, 0)) (NB: the argument is not a string here) raises an AttributeError exception because in this case (i.e. a tuple of invalid length) the validator tries to call the .find method of the argument (which does not exist for tuples). Seems like an issue to me, but I may be wrong.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 16, 2017

I honestly care very little about the rcparam validator because, as I have already argued in many places, I think its entire design should be redone from scratch (to just use python expressions as syntax, either through eval or through a hand-written safe AST evaluator if we consider it worth it).
But you should feel free to push additional commits to this PR to enhance the tests, I won't complain either.

@afvincent
Copy link
Contributor

@anntzer I created a branch on my own fork, which first replicates your commit and second add a proper test for the validator. Unfortunately, I did not manage to understand how to push it to your branch or open a PR against it :/.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 20, 2017

Something like

git fetch upstream pull/9038/head:pull/9038
git checkout pull/9038
# hack hack
git commit
git push

should work. Perhaps this should go to the devdocs, btw.

@afvincent
Copy link
Contributor

Thank you @anntzer, I was severely missing the first line ^^. Almost there now but it looks like I am still missing something for pushing... A simple git push yield

fatal: the current branch  pull/9038 does not have an upstream branch.
To push the current branch and set the remote as upstream, use

    git push --set-upstream origin pull/9038

Of course the suggested solution just made a branch on my own fork, while replacing origin by upstream led to consequences I do not want to think about again 🐑 ...

@anntzer
Copy link
Contributor Author

anntzer commented Aug 20, 2017

What are these consequences? (dunno)
We may as well get how to do this clearly written in the devdocs.

@tacaswell
Copy link
Member

I cherry-picked @afvincent 's second commit and pushed it to this branch.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Aug 20, 2017
@afvincent
Copy link
Contributor

BTW @anntzer, the "consequences" were that I created a new branch on the upstream Matplotlib repository :/.

Huh, seems like I let a white blank line on my initial branch on my own fork, which causes PEP8 failures. Too bad, I had it remove on the branch I created on the upstream repository ^^.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 20, 2017

Sorry about the wrong instructions then :/

@afvincent
Copy link
Contributor

Don't be: thanks to them I already went further than I had before ;).

@dstansby dstansby merged commit c9c6f6b into matplotlib:master Aug 22, 2017
@anntzer anntzer deleted the len-4-color-rcparams branch August 22, 2017 20:18
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.

DOC: better document rcParams in savefig.* grouping
4 participants