Skip to content

Fix cn bugs #6374

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 5 commits into from
May 7, 2016
Merged

Fix cn bugs #6374

merged 5 commits into from
May 7, 2016

Conversation

tacaswell
Copy link
Member

Fixes two bugs:

  • as was noted by @anntzer that the caching mechanism in to_rgb meant that the CN mapping is only done once
  • tweak validation in rcsetup for colors that go into prop_cycle to forbid setting CN style values (due to circularness)

@tacaswell tacaswell added this to the 2.0 (style change major release) milestone May 6, 2016
@@ -29,11 +29,12 @@
import collections as abc
from matplotlib.fontconfig_pattern import parse_fontconfig_pattern
from matplotlib.colors import is_color_like
import re
Copy link
Member

Choose a reason for hiding this comment

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

re should be above matplotlib imports.

Copy link
Member

Choose a reason for hiding this comment

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

This import is still in the wrong location

Copy link
Member Author

Choose a reason for hiding this comment

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

I got the other one

@tacaswell
Copy link
Member Author

fixed typos/import ordering and removed the commit that add the color cycler in rcsetup (as that breaks tests).

I think this should go in (despite @WeatherGod 's protests) to fix the test and we open a new issue to discuss how the default color is handled.

@jenshnielsen
Copy link
Member

Looks like this still have issues on python 2.7

@WeatherGod
Copy link
Member

yeah, those 2.7 failures are non-trivial. Something is seriously wrong.

@WeatherGod
Copy link
Member

I'll build and test this pr for v2.7 to see if I can figure it out

@WeatherGod
Copy link
Member

I can reproduce the two hatch validation errors, and one of the two image comparison failures. The plot for the linestyle cycle test is coming up blank... no lines, no legend. I can't reproduce the markerstyle test failure, though. Makes me wonder if something non-deterministic is happening?

@WeatherGod
Copy link
Member

I got other obligations for today, so I don't have more time to pursue this until tonight.

@tacaswell
Copy link
Member Author

The python2 issue was just using the wrong string type.

It makes me happy to find string/unicode issues that only show up on LPy 😄 .

@efiring
Copy link
Member

efiring commented May 7, 2016

Given that this is passing tests now, I am going to go ahead and merge it so that testing of other PRs will no longer be broken. Work can proceed towards solving #6380.

@efiring efiring merged commit cb5f537 into matplotlib:v2.x May 7, 2016
@efiring
Copy link
Member

efiring commented May 7, 2016

I merged this into master.

@WeatherGod
Copy link
Member

I am still a little concerned about those non-deterministic errors. Could it be a problem with the caching mechanism?

@efiring
Copy link
Member

efiring commented May 8, 2016

@WeatherGod, I thought the remaining Travis error (frequent but intermittent) was just the doc build error we have been seeing recently, which is a failure of Travis to download basemap. However, the attempt by @jenshnielsen to fix that with #6379 failed Appveyor last night on 2.7, apparently because it didn't find latex.

@WeatherGod
Copy link
Member

Nope. As I noted earlier:

and one of the two image comparison failures. The plot for the linestyle cycle test is coming up blank... no lines, no legend. I can't reproduce the markerstyle test failure, though. Makes me wonder if something non-deterministic is happening?

Now, the damnest thing is that I just tried out the branch with the last fix in there (which should have nothing to do with the remaining failure), and it passes -- multiple times. I don't get it.

@tacaswell
Copy link
Member Author

I think the problem is that there is a leak in decorators exception
handling. Because we are running the tests in parallel exactly which test
gets broken is a bit random.

On Sun, May 8, 2016, 13:39 Benjamin Root notifications@github.com wrote:

Nope. As I noted earlier:

and one of the two image comparison failures. The plot for the linestyle
cycle test is coming up blank... no lines, no legend. I can't reproduce the
markerstyle test failure, though. Makes me wonder if something
non-deterministic is happening?

Now, the damnest thing is that I just tried out the branch with the last
fix in there (which should have nothing to do with the remaining failure),
and it passes -- multiple times. I don't get it.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#6374 (comment)

@WeatherGod
Copy link
Member

perhaps, but it isn't limited to parallel execution. Yesterday, I ran only test_cycles.py directly and got that one failure in addition to the two hatch property validation errors. Nothing was parallel. There may be a leak, but it is likely within the same process.

@tacaswell tacaswell deleted the fix_cn_bugs branch May 8, 2016 18:20
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.

5 participants