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

Fixes doubled colors in legend and reduces label count #1895

Merged
merged 10 commits into from May 13, 2016

Conversation

Projects
None yet
3 participants
@chaosphere2112
Contributor

chaosphere2112 commented Mar 25, 2016

Fixes the issues outlined in #1894; still need to add a test.

Sam Fries
@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 6, 2016

@chaosphere2112 when you add a test could you please reopen this again?

@aashish24 aashish24 closed this Apr 6, 2016

chaosphere2112 pushed a commit to CDAT/uvcdat-testdata that referenced this pull request Apr 6, 2016

Sam Fries

@chaosphere2112 chaosphere2112 reopened this Apr 6, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 14, 2016

@chaosphere2112 can you resolve the merge conflicts?

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Apr 14, 2016

@aashish24 Yeah; there's another bug that I ran into that I'm running down right now. I'll get an update for this by EOD.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Apr 14, 2016

Sure, thanks.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Apr 14, 2016

@aashish24 I ran into a few more issues, I'll clean this up in the morning.

Sam Fries added some commits Apr 15, 2016

Sam Fries
Sam Fries
Sam Fries
@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Apr 18, 2016

@doutriaux1 @aashish24

Here's where this PR stands currently on CDASH.

So this is starting to get a little bit out of hand. I've done some major improvements to some of the baselines (test_vcs_boxfill_zero and test_vcs_boxfill_lev1_lev2 are notably better), but then there's... debatable movement on others (test_vcs_boxfill_lev1_lev2_ext1 is a good example of one that isn't bad, but it's certainly not better than it was). I think I fixed a bug in test_vcs_issue_960 (it looks like the last label was missing from the legend, for some reason). How do people feel about less "pretty" numbers for some scales? I can try and be a little smarter about reverting to the old method (vcs.mkscale), but my first attempt at doing so made test_vcs_boxfill_lev1_lev2 revert back to the old legend.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 4, 2016

@chaosphere2112 less pretty numbers you meant they are decimans now? I like the new legend but I think it would be great to have an option to clamp to round numbers since those are easy to read but in some cases you may not want that.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented May 4, 2016

@aashish24 The users still have the ability to use round numbers; if they don't specify level_1 and level_2 on the boxfill, it'll behave exactly the same as it always has (which is why I'm not failing more baselines 😉 ). The "less pretty" numbers are a kind of weird artifact of how I'm generating the labels; looking at the code, I'm guessing there's an off-by-one error somewhere that I've missed which is responsible for an extra label making the levels be shifted off of the "nice" values.

@chaosphere2112 chaosphere2112 referenced this pull request May 9, 2016

Merged

Fix 1894 #129

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented May 9, 2016

@aashish24 @doutriaux1 Alright, this is ready. I fixed the off-by-one errors, so now the same output should be generated for the same input, plus improvements (like a missing level, and lining up the last level, and other goodies).

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented May 10, 2016

@aashish24 ping

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 10, 2016

thanks @chaosphere2112 will look into it today.

low_end = numpy.ma.log10(low_end)
high_end = numpy.ma.log10(high_end)

This comment has been minimized.

@aashish24

aashish24 May 11, 2016

Contributor

@chaosphere2112 nitpick, do we need this many empty lines?

This comment has been minimized.

@chaosphere2112

chaosphere2112 May 11, 2016

Contributor

Nope, I'll remove the extra one.

This comment has been minimized.

@aashish24
numpy.ma.power(
10.,
idigleft))))
idig = numpy.ma.maximum(idig, numpy.ma.floor(numpy.ma.log10(aa * numpy.ma.power(10., idigleft))))

This comment has been minimized.

@aashish24

aashish24 May 11, 2016

Contributor

can we wrap this long line?

This comment has been minimized.

@chaosphere2112

chaosphere2112 May 11, 2016

Contributor

Our max line length is 120, this line is 106, so, no. This is way more readable than the crazy line wrapping I replaced.

This comment has been minimized.

@aashish24

aashish24 May 13, 2016

Contributor

you are right @doutriaux1 did push us for 120 😄

This comment has been minimized.

@doutriaux1

doutriaux1 May 13, 2016

Member

exactly for this kind of lines. I'm a visionary @aashish24

This comment has been minimized.

@aashish24

aashish24 May 13, 2016

Contributor

exactly for this kind of lines. I'm a visionary @aashish24

@doutriaux1 you are a ⭐️ too 😄

@@ -286,7 +245,7 @@ def _plotInternalBoxfill(self):
# Colortable bit
# make sure length match
numLevels = len(self._contourLevels)
numLevels = len(self._contourLevels) - 1

This comment has been minimized.

@aashish24

aashish24 May 11, 2016

Contributor

Why -1 now?

This comment has been minimized.

@chaosphere2112

chaosphere2112 May 11, 2016

Contributor

It was wrong before. When we use it right below, we pad the colors in _contourColors to the length of numLevels, but _contourLevels represents the bounds of each "level"; this means that there was one more color than there should have been, and led to a duplicated color appearing at the end of the legend (hard to see if you're using lots of levels, but very easy to see when you drop it down to a small number of levels). That's actually the root of this whole pull request, but I hit on some other sticky widgets along the way.

This comment has been minimized.

@aashish24

aashish24 May 13, 2016

Contributor

ah I see. Thanks @chaosphere2112 for clarification.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 13, 2016

LGTM 👍

@chaosphere2112 chaosphere2112 merged commit 5ba2503 into master May 13, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 13, 2016

@chaosphere2112 don't forget to delete branch after merging

@doutriaux1 doutriaux1 deleted the fix_1894 branch May 13, 2016

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented May 13, 2016

@doutriaux1 Sorry, got excited and merged when I was going to bed 😄

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 13, 2016

you mean you actually take time to sleep! 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment